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

Refactor server listen to take 'http(s)' scheme in URL #762

Merged
merged 1 commit into from
May 30, 2018

Conversation

JAORMX
Copy link
Contributor

@JAORMX JAORMX commented May 30, 2018

This refactor consists of two things:

  • Accept 'http(s)' as a scheme

  • Move the code so it's easier to add new protocols to --addr

Signed-off-by: Juan Antonio Osorio Robles jaosorior@redhat.com

This refactor consists of two things:

* Accept 'http(s)' as a scheme

* Move the code so it's easier to add new protocols to --addr

Signed-off-by: Juan Antonio Osorio Robles <jaosorior@redhat.com>
Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

Looks good to me -- the thing I'm not sure about is that this way, it's either http[s] OR unix socket, but never both. If that's the intended way, 👍

@tsandall
Copy link
Member

This looks good. The only issue I noticed is that the server errors if you supply a URL scheme for --insecure-addr:

torin:~$ opa run --tls-cert-file server.crt --tls-private-key-file server.key --addr :8181 --insecure-addr http://:9191 -s
INFO[2018-05-30T08:05:46-07:00] First line of log stream.                     addr=":8181" insecure_addr="http://:9191"
FATA[2018-05-30T08:05:46-07:00] Listener failed.                              err="listen tcp: address http://:9191: too many colons in address"

(Specifying a host in the --insecure-addr URL does not change anything.)

@JAORMX
Copy link
Contributor Author

JAORMX commented May 30, 2018

@tsandall right, this only applies for addr and not insecure-addr. I'm thinking in a further iteration we could deprecate insecure-addr? I think this should be all handled by addr (since it takes http and https).

@tsandall
Copy link
Member

tsandall commented May 30, 2018

Deprecating --insecure-addr sounds good to me. We can leave --insecure-addr without support for URL schemes then.

I'll create a separate issue to deprecate --insecure-addr.

I've tried to summarize the new behaviour below. Let me know if this makes sense.

scheme cert provided listener
none no http
none yes https
http no http
http yes http
https no error
https yes https
unix n/a unix
other n/a error

This means that if you want the current https + insecure-addr behaviour you would run OPA as follows (e.g., HTTPS on 0.0.0.0:8181 and HTTP on localhost:8282):

--tls-cert-file xxx --tls-private-key-file yyy --addr :8181 --addr http://localhost:8282

Or alternatively, with URL scheme:

--tls-cert-file xxx --tls-private-key-file yyy --addr https://0.0.0.0:8181 --addr http://localhost:8282

@tsandall tsandall merged commit 565786b into open-policy-agent:master May 30, 2018
tsandall added a commit to tsandall/opa that referenced this pull request May 30, 2018
With the open-policy-agent#762 and open-policy-agent#764 there is no need to keep the logic for creating
the HTTP and HTTPS listeners inside the function. These changes just
split listener creation into separate functions for clarity.

Also, update runtime to use logrus for reporting initialization errors
instead of println for consistency.

Signed-off-by: Torin Sandall <torinsandall@gmail.com>
tsandall added a commit that referenced this pull request May 30, 2018
With the #762 and #764 there is no need to keep the logic for creating
the HTTP and HTTPS listeners inside the function. These changes just
split listener creation into separate functions for clarity.

Also, update runtime to use logrus for reporting initialization errors
instead of println for consistency.

Signed-off-by: Torin Sandall <torinsandall@gmail.com>
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