Skip to content

etcd: Connection options (socket file, ipv6)#468

Merged
purpleidea merged 1 commit intopurpleidea:masterfrom
aequitas:standalone
Feb 13, 2019
Merged

etcd: Connection options (socket file, ipv6)#468
purpleidea merged 1 commit intopurpleidea:masterfrom
aequitas:standalone

Conversation

@aequitas
Copy link
Copy Markdown
Contributor

I know some changes to the etcd code are upcoming. But these changes might be simple enough to already incorporate.

Changes:

  • Allow unix domain socket to be used as client url
  • Using ::1 as clienturl should not create default local ipv4 listener: http://[::1]:
  • Adds no-network-standalone option which starts the server without any TCP ports open

The reason behind this is that open TCP ports make the mgmt daemon vulnerable as long as authentication is not implemented. Using unix:// sockets allows authorization using file system permissions and allows to run a isolated mgmt daemon to run safely. By default the sockets for standalone are created in the prefix directory which should be secured anyway.

This feature was originally posted as: #343

In that PR there where still issues with grpc. Those have been solved in the upstream.

@purpleidea
Copy link
Copy Markdown
Owner

Hey, I'm happy to review and merge what I can, I just can't promise which parts of this will get preserved in my upcoming re-write. (I'll try and incorporate as many as possible into the rebase, although my lack of understanding with parts of this might require that you re-submit anything I've missed in the future.)

Having said that, if you're okay with that, review is coming...

Copy link
Copy Markdown
Owner

@purpleidea purpleidea left a comment

Choose a reason for hiding this comment

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

Here's the review! Thanks!

@aequitas
Copy link
Copy Markdown
Contributor Author

Sound good to me. I'll jump in if you push the rewrite code to adapt where needed for this feature. But I don't expect much issues anyways as nothing fundamental is changed in this PR, only that more URL types (which etcd already supports) are allowed to be passed.

@aequitas aequitas force-pushed the standalone branch 6 times, most recently from 08b90b7 to 3b1bb1d Compare February 12, 2019 22:19
Copy link
Copy Markdown
Owner

@purpleidea purpleidea left a comment

Choose a reason for hiding this comment

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

One small fix needed, and we need to move the chdir stuff out for now. Will this still work without the chdir part in?

@aequitas
Copy link
Copy Markdown
Contributor Author

All chdir is removed. The feature is less convenient now but should still provide the core functionality needed.

@aequitas
Copy link
Copy Markdown
Contributor Author

The build is currently failing on the ipv6-localhost tests. I guess this is due to TravisCI not having ipv6 enabled, but I'm still investigating.

@aequitas aequitas force-pushed the standalone branch 2 times, most recently from 63bdcaf to 34fdddb Compare February 13, 2019 15:22
@aequitas
Copy link
Copy Markdown
Contributor Author

I disabled the test for hosts that don't have ipv6 localhost.

Copy link
Copy Markdown
Owner

@purpleidea purpleidea left a comment

Choose a reason for hiding this comment

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

Patch looks perfect, thanks! I found a few nits to change. I'm only pointing them out because the missing newline was important, so you get them all.

Thanks again!

- Allow unix domain socket to be used as client url
- Using ::1 as clienturl should not create default local ipv4 listener
- Add shell tests
@aequitas
Copy link
Copy Markdown
Contributor Author

That should be everything I think.

@purpleidea purpleidea merged commit f7a06c1 into purpleidea:master Feb 13, 2019
@purpleidea
Copy link
Copy Markdown
Owner

Looks perfect! Thank you again, Merged :)

@aequitas
Copy link
Copy Markdown
Contributor Author

Thanks \o/

@aequitas aequitas deleted the standalone branch February 13, 2019 20:44
@purpleidea
Copy link
Copy Markdown
Owner

Thank you!

(If you feel like elaborating on where the chdir was needed-- (I'm guessing in etcd?) then you could open an upstream issue asking for absolute paths perhaps?)

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.

2 participants