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

Replace base64 encoding with random uuid #1215

Merged
merged 1 commit into from
Aug 22, 2017

Conversation

aledbf
Copy link
Member

@aledbf aledbf commented Aug 22, 2017

No description provided.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 22, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@@ -342,8 +342,7 @@ var (

func buildWhitelistVariable(s string) string {
if _, ok := whitelistVarMap[s]; !ok {
str := base64.URLEncoding.EncodeToString([]byte(s))
whitelistVarMap[s] = strings.Replace(str, "=", "", -1)
whitelistVarMap[s] = fmt.Sprintf("whitelist_%v", uuid.New())
Copy link
Contributor

Choose a reason for hiding this comment

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

You still need to strip the - from the uuid's.

Copy link
Member Author

Choose a reason for hiding this comment

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

that is not needed, we already use this for the whitelist feature

    geo $the_real_ip $deny_9ab24030-716f-4c64-9ed5-ec94ec67f001 {
        default 1;

        192.168.8.104/32 0;
    }

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, I tried that yesterday and got an error because its an invalid variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me test real quick

Copy link
Contributor

Choose a reason for hiding this comment

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

Error: exit status 1
2017/08/22 13:02:25 [emerg] 21#21: unknown "whitelist_8329fc5c" variable
nginx: [emerg] unknown "whitelist_8329fc5c" variable
nginx: configuration file /tmp/nginx-cfg571771066 test failed
 -------------------------------------------------------------------------------
W0822 13:02:25.543292       5 queue.go:90] requeuing kube-system/kube-dns, err 
-------------------------------------------------------------------------------
Error: exit status 1
2017/08/22 13:02:25 [emerg] 21#21: unknown "whitelist_8329fc5c" variable
nginx: [emerg] unknown "whitelist_8329fc5c" variable
nginx: configuration file /tmp/nginx-cfg571771066 test failed
 -------------------------------------------------------------------------------
I0822 13:02:28.050807       5 controller.go:423] backend reload required
E0822 13:02:28.077494       5 controller.go:428] unexpected failure restarting the backend: 
 -------------------------------------------------------------------------------
Error: exit status 1
2017/08/22 13:02:28 [emerg] 26#26: unknown "whitelist_8329fc5c" variable
nginx: [emerg] unknown "whitelist_8329fc5c" variable
nginx: configuration file /tmp/nginx-cfg523483857 test failed
 -------------------------------------------------------------------------------
W0822 13:02:28.077531       5 queue.go:90] requeuing kube-system/kubernetes-dashboard, err 
-------------------------------------------------------------------------------
Error: exit status 1
2017/08/22 13:02:28 [emerg] 26#26: unknown "whitelist_8329fc5c" variable
nginx: [emerg] unknown "whitelist_8329fc5c" variable
nginx: configuration file /tmp/nginx-cfg523483857 test failed
 -------------------------------------------------------------------------------```

Copy link
Member Author

Choose a reason for hiding this comment

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

@sethpollack please post the ingress you are using

Copy link
Contributor

Choose a reason for hiding this comment

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

kind: Ingress
metadata:
  name: sample-app
  annotations:
    kubernetes.io/ingress.class: "nginx"
    ingress.kubernetes.io/limit-whitelist: 127.0.0.1
    ingress.kubernetes.io/limit-rpm: "10"
spec:
  rules:
    - host: sample-app.com
      http:
        paths:
          - path: /foo
            backend:
              serviceName: nginx-default-backend
              servicePort: 80

Copy link
Contributor

Choose a reason for hiding this comment

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

I only get the error with the rate limit whitelist, not ingress.kubernetes.io/whitelist-source-range

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 44.387% when pulling c4fc497 on aledbf:random-name into def5155 on kubernetes:master.

@aledbf
Copy link
Member Author

aledbf commented Aug 22, 2017

@sethpollack please review. Temporal image with this PR: quay.io/aledbf/nginx-ingress-controller:0.188

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.004%) to 44.416% when pulling a392f29 on aledbf:random-name into def5155 on kubernetes:master.

Copy link
Contributor

@sethpollack sethpollack left a comment

Choose a reason for hiding this comment

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

lgtm

@aledbf aledbf closed this Aug 22, 2017
@aledbf aledbf reopened this Aug 22, 2017
@aledbf aledbf merged commit ed3803c into kubernetes:master Aug 22, 2017
@sethpollack
Copy link
Contributor

Thanks!

@aledbf aledbf deleted the random-name branch August 22, 2017 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants