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

Add cors #144

Closed
wants to merge 11 commits into from
Closed

Add cors #144

wants to merge 11 commits into from

Conversation

cheddarwhizzy
Copy link

No description provided.

@cheddarwhizzy cheddarwhizzy mentioned this pull request May 22, 2017
@pleshakov
Copy link
Contributor

@bporter2387
Thanks!

I have a couple of suggestions/improvements:

  1. It is better to move CORS add_header directives from the location block to the server block, as currently, CORS add_header directives conflict with HSTS add_header directives, defined in the server block. When add_header directives are defined both in a server and in a location block, the ones from the location block overwrite the ones from the server block.
  2. OPTIONS method should be allowed for all origins. I think that's why your upstreams are still hit from non-whitelisted Origins - because the non-whitelisted Origins are not able to make the OPTIONS request. In this case, returning 401 becomes unnecessary.
  3. add_header 'Access-Control-Allow-Headers' .. should be present for non-OPTIONS requests as well, according to https://enable-cors.org/server_nginx.html
  4. . in the domains must be escaped in the regex.
  5. There is another server where CorsEnabled and CorsEnabled must be added.

Brett Porter and others added 3 commits May 30, 2017 11:48
@cheddarwhizzy
Copy link
Author

cheddarwhizzy commented May 30, 2017

Hey, thanks for the feedback! I corrected 2-5, however I'm unsure how to go about number 1 since add_header isn't allowed in the server block when inside an 'if' statement. Any suggestions there?

@pleshakov
Copy link
Contributor

@bporter2387

It is important to be able to properly add CORS to the Ingress controller, so thanks for your pull request! However, I've been thinking more about this feature and it doesn't look like the two annotations -- nginx.org/enable-cors and nginx.org/cors-domains cover most of the CORS use-cases. For example, depending on an application, the user might want to customize:

  • Access-Control-Allow-Methods
  • Access-Control-Allow-Headers
  • Access-Control-Expose-Headers
  • Access-Control-Allow-Credentials
  • Access-Control-Max-Age
  • the behavior when a request comes from an origin that is not allowed. For example, the user might want to return a 401 with a custom error page.
  • other possible customizations.

To be able to customize those items, the user will have to change the ingress.tmpl, which undermines the value of having CORS annotations.

We can try to provide the necessary level of customization by adding additional annotations. However, I don't think it would be the best solution, because CORS requirements can be quite different and it is possible it would require adding more and more annotations to cover those requirements.

As an alternative, I can suggest the following solution that utilizes the ConfigMaps resource and the nginx.org/location-snippets annotations:

Step 1. Create a configMap with the CORS configuration

Step 1.1. Create a file named cors.conf with the following content (the content is based on your configuration from ingress.tmpl, but for simplicity, it doesn't have the origin check):

set $cors "true";
set $corsmethod "${cors}nonoptions";

if ($request_method = 'OPTIONS') {
    set $corsmethod "${cors}options";
}

if ($corsmethod = "truenonoptions") {
    add_header 'Access-Control-Allow-Origin' "$http_origin" always;
    add_header 'Access-Control-Allow-Credentials' 'true' always;
    add_header 'Access-Control-Allow-Methods' 'GET, POST, PATCH, PUT, DELETE, OPTIONS' always;
    add_header 'Access-Control-Allow-Headers' 'Authorization,Content-Type,Accept,Origin,User-Agent,DNT,Cache-Control,X-Mx-ReqToken,Keep-Alive,X-Requested-With,If-Modified-Since';
    add_header 'Access-Control-Expose-Headers' 'DNT,X-CustomHeader,Keep-Alive,User-Agent,X-Requested-With,If-Modified-Since,Cache-Control,Content-Type,Content-Range,Range';
}

if ($corsmethod = "trueoptions") {
    add_header 'Access-Control-Allow-Origin' "$http_origin";
    add_header 'Access-Control-Allow-Credentials' 'true';
    add_header 'Access-Control-Max-Age' 1728000;
    add_header 'Access-Control-Allow-Methods' 'GET, POST, PATCH, PUT, DELETE, OPTIONS';
    add_header 'Access-Control-Allow-Headers' 'Authorization,Content-Type,Accept,Origin,User-Agent,DNT,Cache-Control,X-Mx-ReqToken,Keep-Alive,X-Requested-With,If-Modified-Since';
    add_header 'Content-Length' 0;
    add_header 'Content-Type' 'text/plain charset=UTF-8';
    return 204;
}

Step 1.2. Create a ConfigMap resource from cors.conf:

$ kubectl create configmap cors-config --from-file=./cors.conf

Step 2. Deploy the Ingress controller and mount the ConfigMap resource on the file system, using the following manifest:

apiVersion: v1
kind: ReplicationController
metadata:
  name: nginx-ingress-rc
  labels:
    app: nginx-ingress
spec:
  replicas: 1
  selector:
    app: nginx-ingress
  template:
    metadata:
      labels:
        app: nginx-ingress
    spec:
      containers:
      - image: nginx-ingress:0.8.1
        imagePullPolicy: Always
        name: nginx-ingress
        ports:
        - containerPort: 80
          hostPort: 80
        - containerPort: 443
          hostPort: 443
        args:
         - -v=3
        volumeMounts:
        - name: config-volume
          mountPath: /etc/nginx/custom-snippets/
      volumes:
        - name: config-volume
          configMap:
            name: cors-config

Note that cors.conf file will be placed in the /etc/nginx/custom-snippets folder.

Step 3. Using the nginx.org/location-snippets annotation, include cors.conf in location blocks of your Ingress resource:

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: cafe-ingress
  annotations:
    kubernetes.io/ingress.class: "nginx"
    nginx.org/location-snippets: "include /etc/nginx/custom-snippets/cors.conf;"
spec:
  rules:
  - host: cafe.example.com
    http:
      paths:
      - path: /tea
        backend:
          serviceName: tea-svc
          servicePort: 80
      - path: /coffee
        backend:
          serviceName: coffee-svc
          servicePort: 80

Create the Ingress resource.
The generated config will lool like the following:

...
       location /tea {
                proxy_http_version 1.1;
                include /etc/nginx/custom-snippets/cors.conf;
...

Using the ConfigMap resource and the nginx.org/location-snippets annotation allows you to easily add complex NGINX configuration snippets to your Ingress resources. Alternatively, it is possible to put the content of cors.conf into the nginx.org/location-snippetsas shown here.

Please let me know your thoughts. Does the ConfigMaps/location-snippet approach work for you? Do you see any disadvantages?
I appreciate the work you've done for this pull request. However, before proceeding further, it is important to make sure that we are creating an optimal solution for users.

@cheddarwhizzy
Copy link
Author

I think this is a good solution. You're right, too many hard-coded values. Give me a few days and I'll see what I can come up with.

@cheddarwhizzy
Copy link
Author

After using the location snippets annotation (which I didn't know of prior to this PR), I like this approach much better as it allows for much better customization vs the code in this PR. I'll go ahead and close this out. Thanks for the tips!

@pleshakov
Copy link
Contributor

Thanks

@dolftax
Copy link

dolftax commented Jul 25, 2017

Is there any update here? (or) alternatives?

@pleshakov
Copy link
Contributor

@dolftax
To enable CORS, I suggest using the approach from this comment -- #144 (comment)

Alternatively, you can use the kubernetes/ingress NGINX controller, it provides an annotation for enabling CORS --
https://github.com/kubernetes/ingress/blob/a58b80017170eecbe8b2d6573b66192cafe0d32a/controllers/nginx/configuration.md#enable-cors However, the generated CORS config might not work for all cases, so I recommend creating your own CORS configuration and configure NGINX as described in the comment -- #144 (comment)

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.

3 participants