Skip to content
This repository has been archived by the owner on Mar 9, 2022. It is now read-only.

Serve streaming on localhost by default to match k8s 1.11 default. #858

Merged
merged 1 commit into from Jul 21, 2018

Conversation

Random-Liu
Copy link
Member

@Random-Liu Random-Liu commented Jul 20, 2018

Ref #777.

Change the default config to match Kubernetes 1.11 default behavior.

Initially, I thought we should keep default unchanged so that the default config can work with both K8s 1.10 and 1.11.

However, I talked with @dmcgowan today, and we found users don't like arbitrary fixed port listening. Given that we don't need a fixed port anymore for K8s 1.11, I think we should change the default to match the k8s 1.11 behavior to get rid of this fixed port.

We need to mention in the release note that, if you want to use containerd 1.2 with K8s 1.10, please set stream_server_address="" and stream_server_port to a fixed port if you need a fixed one.

Signed-off-by: Lantao Liu lantaol@google.com

Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

Agree arbitrary not as good as specified port.. more secure.

@Random-Liu
Copy link
Member Author

Agree arbitrary not as good as specified port.. more secure.

Just to clarify, with this PR, we are going to use arbitrary (non-fixed) port on localhost. Because it is localhost, there shouldn't be security problem ideally.

@mikebrow
Copy link
Member

Sry was typing fast. I meant Agree with arbitrary point, but specified is more secure. Release note and maybe a note in the config.md will suffice.

@mikebrow
Copy link
Member

Which reminds me.. can we update the comment for this up on line 105- and the config.md?

@Random-Liu
Copy link
Member Author

@mikebrow Make sense... Will do! Thanks for reminding!

Signed-off-by: Lantao Liu <lantaol@google.com>
@Random-Liu
Copy link
Member Author

@mikebrow Done.

Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

/LGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants