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 rewrites annotation #64

Merged
merged 1 commit into from Nov 8, 2016

Conversation

jmastr
Copy link

@jmastr jmastr commented Nov 7, 2016

Closes #63

@jmastr
Copy link
Author

jmastr commented Nov 8, 2016

@pleshakov please review

@pleshakov
Copy link
Contributor

@jmastr
Thx for another pull request!

A few suggestions:

  • Remove {{if $location.Rewrite}}/{{end}} from location {{$location.Path}}{{if $location.Rewrite}}/{{end}} in the ingress.tmpl. I think it's better to stay transperent. I'll add notes about the necessity to put / at the end of Ingress paths it to the README file.
  • Don't validate the service name and the URL in the annotation.
r, _ := regexp.Compile("serviceName=([a-z\\-]+) +rewrite=([/a-z0-9]+)")

A service name can also contain numbers. A URL can contain characters like _. I suggest to not perform validation at all. If a service name is not valid, the controller will simply igoner the service later. If a URL is not correct, NGINX will report error in the logs when the controller does the configuration check before reloading the configuration.

Please look at https://github.com/nginxinc/kubernetes-ingress/blob/master/nginx-plus-controller/nginx/configurator.go#L232 , where we're parsing a similar annotation.

@jmastr
Copy link
Author

jmastr commented Nov 8, 2016

Very nice! Will do!

@jmastr jmastr force-pushed the jmastr/add_rewrites_annotation branch from bcaa65a to 4456fb3 Compare November 8, 2016 13:19
@jmastr
Copy link
Author

jmastr commented Nov 8, 2016

Hi @pleshakov,

I worked in your comments about parsing, logging and testing.

Note that I removed the / at the end of location {{$location.Path}} from my patch, but I added it to examples/rewrites/README.md already to be consistent.

@pleshakov
Copy link
Contributor

@jmastr
Awesome!

Could you replace nginx.com with nginx.org in the following line in configurator.go : glog.Errorf("In %v nginx.com/rewrites contains invalid declaration: %v, ignoring", ingEx.Ingress.Name, err) ?

@jmastr jmastr force-pushed the jmastr/add_rewrites_annotation branch from 4456fb3 to 4c0f7f8 Compare November 8, 2016 16:23
@jmastr
Copy link
Author

jmastr commented Nov 8, 2016

@pleshakov done

@pleshakov
Copy link
Contributor

Great. Thx

@pleshakov pleshakov merged commit 57918f6 into nginxinc:master Nov 8, 2016
@pleshakov
Copy link
Contributor

We'll prepare a new release -- https://github.com/nginxinc/kubernetes-ingress/releases -- that will have this and other features. A new tag will be published to the https://hub.docker.com/r/nginxdemos/nginx-ingress/ repo.

@jmastr
Copy link
Author

jmastr commented Nov 8, 2016

That's awesome! Thank you very, very much!

@jmastr
Copy link
Author

jmastr commented Nov 8, 2016

I am going to give a talk about this at the next Kubernetes meetup in Berlin, Germany.

@pleshakov
Copy link
Contributor

That's awesome!!

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.

None yet

2 participants