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

Quick fixes for interception and debugging #11065

Merged
merged 32 commits into from Jan 24, 2019

Conversation

@costinm
Copy link
Contributor

commented Jan 17, 2019

  • add none to inject validation
  • use same base as Pilot for Galley - can be switched back to scratch after we make sure it's stable and
    prod ready, but for now we seem to need debugging tools ( see the crash loop bug)

costinm added some commits Jan 17, 2019

@googlebot googlebot added the cla: yes label Jan 17, 2019

@istio-testing istio-testing requested review from geeknoid and kyessenov Jan 17, 2019

@costinm costinm requested review from mandarjog, rshriram and ozevren and removed request for kyessenov and geeknoid Jan 17, 2019

@ozevren

This comment has been minimized.

Copy link
Member

commented Jan 18, 2019

/lgtm

costinm added some commits Jan 19, 2019

@costinm costinm requested a review from duderino Jan 19, 2019

.

@rshriram

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

/hold

@rshriram

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

I looked at the Gateway pods. Envoy in the gateway pod's docker container runs as a root process allowing it to be bound to port 80 or other priviledged ports. This does not mean the pod needs priviledged access. Its just the container port [within Docker]. So, if a user could run normal processes bound to port 80 in a strictly controlled k8s environment (e.g., nginx ingress, etc.), then she should be able to do the same for sidecar docker containers. OTOH, if there is a security policy that no docker container's process should ever listen on priviledged ports (i.e. container ports <1024), then gateway should be fixed in this PR as well. But if you take that route of remapping gateway server's ports as well, you will break all the gateway definitions, virtual service matches, etc.

I personally don't think this is an issue because you probably tested the system with the istio-proxy container in the sidecar without removing the runAsUser / runAsGroup parameters that restricts Envoy to run with GID/PID 1337. For None mode, you should actually tweak the helm templates to remove that 1337 parameter https://github.com/istio/istio/pull/11065/files#diff-de5983fcb0f040db64cf0281f3b152c0R209 - this will eliminate the need for any port remapping (the metadata or the function that adds 15200 to port 80).

Without that UID parameter, Envoy containers run as standard root processes within the docker container and can happily bind to port 80, etc. [remember this has nothing to do with the pod being privileged].

number: 7071
protocol: HTTP
name: httplocal
default_endpoint: 127.0.0.1:17071

This comment has been minimized.

This comment has been minimized.

Copy link
@costinm

costinm Jan 23, 2019

Author Contributor

both work AFAIK. Probably defaultEndpoint is the correct one.

@costinm

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2019

'none' mode also applies to VMs, where running a server as root is really uncommon.

I don't think running as root on a container is a good practice either. The PR does allow users to run as root - but it can't be a requirement, since it's generally to be avoided.

We need to run Gateway as root for this purpose, since Gateway typically binds to 80/443. This PR doesn't touch Gateway anyways, applies only to sidecars.

For Sidecar - we currently run as non-root, and I see no reason to change the default. If a user chooses to take the risks - no problem, they can override the remapping.

@costinm

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2019

Ok, removed the translation for now. We'll document that ports <1024 are not supported unless they run as root or have the right capabilities - since this is mostly a problem for egress it can be addressed by using explicit ports.

@@ -19,6 +19,7 @@ import (
"fmt"
"reflect"
"sort"
"strconv"

This comment has been minimized.

Copy link
@golangcibot

golangcibot Jan 23, 2019

File is not goimports-ed (from goimports)

@rshriram

This comment has been minimized.

Copy link
Member

commented Jan 24, 2019

/hold cancel

costinm added some commits Jan 24, 2019

Temporary revert remapping, since istio-system can't be imported sele…
…ctively, will be removed after needed fixes are merged
@costinm

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2019

On remapping: added a comment, for now until egress is fully implemented ( i.e. allow multiple imports from same ns ) - there is no way to use the system without remapping. Istio-system defines 443 (ingress) and other ns may do the same. Once the bugs in egress are fixed we can remove this.

@rshriram
Copy link
Member

left a comment

okay with the UID idea. Please fix the validPort function (its got some leftover code).

costinm added some commits Jan 24, 2019

@istio-testing

This comment has been minimized.

Copy link
Collaborator

commented Jan 24, 2019

@costinm: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
prow/istio-integ-k8s-tests.sh d54b290 link /test istio-integ-k8s-tests
prow/istio-pilot-multicluster-e2e.sh d54b290 link /test istio-pilot-multicluster-e2e

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@wenchenglu wenchenglu merged commit ac78a55 into istio:release-1.1 Jan 24, 2019

30 of 33 checks passed

prow/istio-integ-k8s-tests.sh Job failed.
Details
prow/istio-pilot-multicluster-e2e.sh Job failed.
Details
tide Not mergeable. Needs lgtm label.
Details
GolangCI No issues found!
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: codecov Your tests passed on CircleCI!
Details
ci/circleci: e2e-dashboard Your tests passed on CircleCI!
Details
ci/circleci: e2e-galley Your tests passed on CircleCI!
Details
ci/circleci: e2e-mixer-noauth-v1alpha3-v2 Your tests passed on CircleCI!
Details
ci/circleci: e2e-pilot-auth-v1alpha3-v2 Your tests passed on CircleCI!
Details
ci/circleci: e2e-pilot-cloudfoundry-v1alpha3-v2 Your tests passed on CircleCI!
Details
ci/circleci: e2e-pilot-noauth-v1alpha3-v2 Your tests passed on CircleCI!
Details
ci/circleci: e2e-simple Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: racetest Your tests passed on CircleCI!
Details
ci/circleci: shellcheck Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: test-integration-local Your tests passed on CircleCI!
Details
cla/google All necessary CLAs are signed
prow/e2e-bookInfoTests-v1alpha3.sh Job succeeded.
Details
prow/e2e-bookInfoTests.sh Skipped
prow/e2e-dashboard.sh Job succeeded.
Details
prow/e2e-mixer-no_auth.sh Job succeeded.
Details
prow/e2e-simpleTests-cni.sh Skipped
prow/e2e-simpleTests-minProfile.sh Job succeeded.
Details
prow/e2e-simpleTests.sh Job succeeded.
Details
prow/e2e_pilotv2_auth_sds.sh Job succeeded.
Details
prow/istio-integ-local-tests.sh Job succeeded.
Details
prow/istio-pilot-e2e-envoyv2-v1alpha3.sh Job succeeded.
Details
prow/istio-pilot-e2e.sh Skipped
prow/istio-presubmit.sh Job succeeded.
Details
prow/istio-unit-tests.sh Job succeeded.
Details
prow/release-test.sh Job succeeded.
Details

exiaohao pushed a commit to exiaohao/istio that referenced this pull request Feb 11, 2019

Quick fixes for interception and debugging (istio#11065)
* Additional fixes for interception

* Use the constant

* Skip init container for none

* Fix 2 crashes for pods without services

* Any priv port causes envoy to reject all listeners

* Matching target port, partial solution by name

* Similar fix for listeners, common method

* Format

* Add TODO

* Use target port from endpoint

* Revert target port - can use the one from instance

* Update the test file and golden for none, make it executable in real cluster

* Fix test (ports now need to match)

* v2 is tested via e2e

* No clue why make format and lint don't agree

* Add a metadata to control port remapping for bind=true and non-root

* Remove remapping of port. User can either run as root or not use <1024

* Format

* Temporary revert remapping, since istio-system can't be imported selectively, will be removed after needed fixes are merged

* Move port mapping to new code path

* Move the search by port name after search by port number

* Replace remap with validate

* Add mixer imports to template

* Use custom fortio image

* Fix  the test after adding mixer inports
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.