The envoy image also installed iptables and added:

ADD istio-iptables.sh /usr/local/bin/istio-iptables.sh
ADD istio-start.sh /usr/local/bin/istio-start.sh

are these not needed?

I don't believe so ... those were the old iptables scripts. The current iptables are set up by the init container and uses the script from istio/istio.

Do we really need something as big as xenial ?

Just being consistent with what the image was previously. Changing it would seem beyond the scope of this PR.

Please add the iptables and istio-start - the docker image can also be used as a basis and outside k8s.

It would be great if this PR copied exactly the dockerfile used in proxy - and we can have a
separate PR if we want to make changes.

ISTIO_OUT might be more appropriate.

I doubt these exports are going to do much good when run via make. The only way I could see these carry over to the other scripts that consume them is if someone runs bin/init.sh directly followed by other commands.

Regardless, istio.deps probably needs to be updated to point to this file on what to update.

Some of these vars could probably used/consumed by the ones in the prior block.

I think tar can be told to extract the one file of interest (probably as "./usr/local/bin/envoy").

There is only one file anyway.

I don't see how the vars from bin/init.sh ever get seen here. I do have a pending change that ports bin/init.sh into the top-level Makefile, though I'm still hung on on how/what to use to perform the download (my initial draft was to use compile an app from a short piece of go code [which was shorter and easier than the curl vs. wget detection] though Laurent suggested I try leveraging fortio instead]).

master...mattdelco:master20

@mattdelco The vars from Makefile are seen here, not init.sh. Currently we copy the vars in the Makefile and init.sh .... eventually we should either merge init.sh into the Makefile or require that it be run from the Makefile.

move the sha to a different variable
+ISTIO_ENVOY_DEFAULT_VERSION="54077a0c8486b6db7b2fc5d10619b6813bb40767"
+export ISTIO_ENVOY_VERSION=${ISTIO_ENVOY_VERSION:-ISTIO_ENVOY__DEFAULT_VERSION}

makes it easier for the update bot

This duplication in logic (both existing, and now expanded) is getting a bit onerous and is a pain to maintain. I wouldn't mind if we changed init.sh so it complained and failed if it isn't run via make (though I also wouldn't mind getting rid of the file in its entirety).

I would avoid bots editing this file.

Can you source istio.VERSION or some other file that is already updated by the bot ?

can you add the 2 lines about how to apt-get that were in the original file

done

This can be reduced to:

docker.proxy: $(ISTIO_DOCKER)/envoy
docker.proxy_debug: $(ISTIO_DOCKER)/envoy-debug

Done.

How important is it to be able to specify/customize the envoy paths? If envoy and envoy-debug was stored in ${ISTIO_OUT} rather than go/out then you wouldn't need these custom rules and could instead add "envoy" and "envoy-debug" to the list for DOCKER_FILES_FROM_ISTIO_OUT. Actually, you'd also have to change this in the top-level Makefile to make knows how to resolve the envoy-debug:

- ${ISTIO_OUT}/envoy: init
+ ${ISTIO_OUT}/envoy ${ISTIO_OUT}/envoy-debug: init

The main problem with my suggestion is that ${ISTIO_OUT}/envoy is currently being used for some tests, so you'd have to do some seek-and-update to change such uses of "envoy" to "envoy-debug". The other tradeoff of my suggestion is that is someone changes their cross-compile target then it'll re-download envoy for each, but in the long run maybe this is ideal for if/when istio gets to the point of compiling proxy for other platforms.

Maybe envoy_debug would be better placed under OUT/$ARCH/debug ?

In general, the current docker building is extremely inefficient - all binaries and files are passed to the docker builder ( copied ), when for most images only a tiny binary is needed.
Placing even more large binaries in the single common dir will make it worse.

Good suggestions. I've changed the directories to be placed in ${OUT_DIR}/$[OS}_${ARCH}/<release/debug>

This duplication in logic (both existing, and now expanded) is getting a bit onerous and is a pain to maintain. I wouldn't mind if we changed init.sh so it complained and failed if it isn't run via make (though I also wouldn't mind getting rid of the file in its entirety).

I would remove the TODO - we do want more tests to use the envoy binary directly (faster,
matches rawvm env etc)!

Please add the iptables and istio-start - the docker image can also be used as a basis and outside k8s.

It would be great if this PR copied exactly the dockerfile used in proxy - and we can have a
separate PR if we want to make changes.

No longer used.

removed.

AFAIK no longer used.

removed.

Maybe envoy_debug would be better placed under OUT/$ARCH/debug ?

In general, the current docker building is extremely inefficient - all binaries and files are passed to the docker builder ( copied ), when for most images only a tiny binary is needed.
Placing even more large binaries in the single common dir will make it worse.

I agree with not peppering the code with SHAs and the point about not duplicating makefile logic

can we put at the beginning "if !RAN_THROUGH_MAKE echo "please type 'make init' to invoke this;" exit 1" equiv

and in Makefile do RAN_THROUGH_MAKE=1 bin/init.sh

Updated to use PROXY_TAG from istio.VERSION.

Hrm, in a different PR I came to see that people (and thus the istio source) seem to have different and conflicting impressions on what PROXY_TAG refers to. Some cases treat this tag as pointing to the envoy images from the proxy repo (e.g., the contents of istio.deps and the files it tells the bots to update), while others cases seem to think this points at the proxy image from the istio repo under pilot/docker (e.g., the -a behavior of install/updateVersion.sh, which sets pilot, proxy, mixer, and ca to all the same values). So, the first time that someone builds it'll probably do okay, but if someone runs updateVersion.sh (e.g., via "make installgen" or "make push") then I'm expecting things will break during a subsequent rebuild because updateVersion.sh will probably update PROXY_TAG in istio.VERSION to point to the istio repo rather than the proxy repo.

@mattdelco where do you see PROXY_TAG referencing the pilot Dockerfiles?

The PROXY_TAG is referenced in files under install/ and samples/ . A clearer sign of the conflicting use is:

samples/bookinfo/eureka/templates/bookinfo.sidecars.yaml.tmpl:    image: {PILOT_HUB}/proxy_debug:{PROXY_TAG}

Just to be clear, this conflicting use of PROXY_TAG existed prior to your change and the -a behavior is arguably broken and/or there needs to be two sets of proxy tags. Prior to your change updateVersion.sh would update the PROXY_TAG in istio.VERSION with a istio repo commit hash but I doubt anyone or anything cared. After your change the build now looks at istio.VERSION so the first "make push" would probably run okay but the second is likely to fail. The main workaround I can see (to avoid being on the hook to fix this preexisting mess) is to store the commit SHA elsewhere (e.g., the Makefile) and have an entry in istio.deps to update that location. One of the downsides of both Makefile and istio.deps is they're fairly high-traffic files (with regards to rate of code changes) and it'll be harder to cook up a make rule that efficiently only downloads a new envoy tar when the sha has changed (since these files often change for other reasons, unlike the Dockerfiles).

Here's the other PR I had in mind:

#3641

IMO it went after the symptom rather than the problem by changing the script to look at istio.deps rather than istio.VERSION (because istio.VERSION is getting changed by updateVersion.sh to point PROXY_TAG at pilot's proxy[_debug] images instead of proxy's envoy images).

removing source of truth from machine parsable json to a shell script is going backwards.

Is it much easier to update istio.deps and everyone else reads dependencies from it.

@mandarjog I don't disagree ... although my understanding of all of this machinery is not great. Feel free to roll a follow-on PR and I'd be happy to review.