Add proxy version info to envoy metadata#6854
Conversation
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
|
Thank you for this much saner approach. We can condition mixer plugin based on the version of the proxy asking for the config. |
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
pilot/pkg/kube/inject/mesh.go
Outdated
| fieldRef: | ||
| fieldPath: status.podIP | ||
| - name: ISTIO_META_ISTIO_PROXY_VERSION | ||
| {{ if eq .IstioProxyVersion "" -}} |
There was a problem hiding this comment.
I don't think this will work - or is needed. Once upgrade happens, auto-inject or inject will put the right proxy version and proxy.
@mandarjog - is this code still used ? My understanding was that we now read the config map and this is dead code to be cleaned up ?
costinm
left a comment
There was a problem hiding this comment.
Looks great - but missing 2 files, need values.yaml in both istio and istio-remote to add the istioProxyVersion.
|
Once you add values.yaml: |
|
Noted in slack: we do allow users to use custom images. Thinking about it - it may be a more flexible approach. For the version: it might be something like: 1.0,mixer5.2,myplugin3.4 if we want to |
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
Codecov Report
@@ Coverage Diff @@
## release-1.0 #6854 +/- ##
============================================
- Coverage 71% 71% -<1%
============================================
Files 369 370 +1
Lines 31584 32041 +457
============================================
+ Hits 22228 22534 +306
- Misses 8428 8575 +147
- Partials 928 932 +4
Continue to review full report at Codecov.
|
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
@costinm values.yaml files updated |
| tag: 0.8.latest | ||
|
|
||
| # git SHA or git tag from istio/proxy repo | ||
| istioProxyVersion: 0.8.0 |
There was a problem hiding this comment.
Not sure what that means - 0.8.0 shipped and it's injected, so this will never be a valid combination.
You mean "1.0.0" ? Also the tag above should be 1.0.latest
| tag: nightly-master | ||
|
|
||
| # git SHA or git tag from istio/proxy repo | ||
| istioProxyVersion: 0.8.0 |
istioctl/cmd/istioctl/kubeinject.go
Outdated
| }); err != nil { | ||
| return err | ||
| } | ||
| return errors.New("one of injectConfigFile or injectConfigMapName is required\n" + |
There was a problem hiding this comment.
We seem to still need it, the tests are doing the wrong thing and using the 'builtin defaults' ( which is the only place the hardcoded config should be used ). May be better to split into 2 PR, one adding the env and separate one (in master?) getting rid of the hack and fixing the tests.
pilot/pkg/kube/inject/inject.go
Outdated
| DebugMode bool `json:"debugMode"` | ||
| Mesh *meshconfig.MeshConfig `json:"-"` | ||
| ImagePullPolicy string `json:"imagePullPolicy"` | ||
| IstioProxyVersion string `json:"istioProxyVersion"` |
pilot/pkg/kube/inject/inject_test.go
Outdated
| InitImage: InitImageName(unitTestHub, unitTestTag, false), | ||
| ProxyImage: ProxyImageName(unitTestHub, unitTestTag, false), | ||
| ImagePullPolicy: "IfNotPresent", | ||
| IstioProxyVersion: unitTestProxyVersion, |
There was a problem hiding this comment.
If you want to add a test - add one to check env variables are propagated. This is just one particular env.
pilot/pkg/model/context.go
Outdated
| Metadata map[string]string | ||
| } | ||
|
|
||
| // ProxyVersion indicates the version of the data plane proxy we are talking to. |
There was a problem hiding this comment.
As I mentioned - I'm not sure what is the right way to handle this, the proxy version doesn't really help since it's not something pilot can really process. We don't support a single sidecar that has linear versions - the sidecar can be a custom build with extra modules, from a remote repo using its own versioning.
How about not designing a new API and model in 1.0 branch - and just use the presence of the env variable in 1.0 to differentiate 0.8.0 ( which will never have this set ) ?
There was a problem hiding this comment.
We can still add capabilities by using x.y.z-tags. This is not exposing any api. It’s simply parsing the metadata field into some struct that we can use internally. Plugins are welcome to add additional info and parse it in any way they want. For networking code in open source, we simply need to track the istio versions so that we can emit the right type of Envoy configs for things like filter chain matching on src/dst ip ranges etc. a true version info would be one where Envoy sends list of filters with versions and capabilities it has. This is a simpler version of that. We could do a lot of things with just this.
If we implement unimplemented pieces of Envoy proto, we need to know of the Envoy will respect it or ignore it, in the main networking code. Such things cannot be converted into tags. There would be hundreds of such tags then (eg filterchainipsupport,rediscds,mongofault,etc) one for each thing we add to Envoy. The numbering scheme is a way to mark these features on a linear semver scale, sort of like kernel code.
I don’t want to have dummy env vars that will get removed in next cycle. People could still use new istioctl with 0.8 proxy, or stock Envoy. Using a value in the environment variable transfers the burden to the operator making custom changes to clearly communicate the major istio release with which the proxy is expected to work.
|
On Thu, Jul 5, 2018 at 5:14 PM Shriram Rajagopalan ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pilot/pkg/model/context.go
<#6854 (comment)>:
> // Metadata key-value pairs extending the Node identifier
Metadata map[string]string
}
+// ProxyVersion indicates the version of the data plane proxy we are talking to.
We can still add capabilities by using x.y.z-tags. This is not exposing
any api. It’s simply parsing the metadata field into some struct that we
can use internally. Plugins are welcome to add additional info and parse it
in any way they want. For networking code in open source, we simply need to
track the istio versions so that we can emit the right type of Envoy
configs for things like filter chain matching on src/dst ip ranges etc. a
true version info would be one where Envoy sends list of filters with
versions and capabilities it has. This is a simpler version of that. We
could do a lot of things with just this.
We can do a lot of things with just this, agree. And simply the presence of
this solves the upgrade problem.
But I don't think this is 'simpler version' - it is far more complicated to
guess and generate a good config, at least
in the long term. Short term is a binary decision - 0.8 or 1.0, the content
of the string doesn't really matter, if it
exists it's 1.0 and whatever envoy/mixer we ship in 1.0.
Once pilot start supporting generating config for custom plugins - and
users start using different builds, having
plugins and versions is simpler, less guessing.
I really don't think we have any case where parsing x, y, z would help -
when we find them we can add the helper, but
right now the change is far more complicated than it needs to be. The
'tags' in x.y.z might help - but I don't think
mangling plugin version in the docker tag is right.
If we implement unimplemented pieces of Envoy proto, we need to know of
the Envoy will respect it or ignore it, in the main networking code. Such
things cannot be converted into tags. There would be hundreds of such tags
then (eg filterchainipsupport,rediscds,mongofault,etc) one for each thing
we add to Envoy. The numbering scheme is a way to mark these features on a
linear semver scale, sort of like kernel code.
Kernel code is certainly not doing that - there is a kernel version, but
the .config has individual settings for each compile option.
From 2.3.4 semver you can't figure what filters went in - an semantic
version has a single dimension. Unless we add some
extra complexity mapping a 'version' to a list of features.
I guess we could do that - but then we can just use the sha1 or sha1 + repo
(proxy:SHA for our own builds), and use some config
to match the exact string to the broken list of features.
I don’t want to have dummy env vars that will get removed in next cycle.
People could still use new istioctl with 0.8 proxy, or stock Envoy. Using a
value in the environment variable transfers the burden to the operator
making custom changes to clearly communicate the major istio release with
which the proxy is expected to work.
How ? Assume injection env variable specifies version 1.2.3. Users can
still specify a custom proxy using the annotation.
If they specify any other version beside the one we ship - the env variable
won't match (because it's a global set at deploy
time).
And if they use the new istioctl with the old proxy - it's even worse. We
would have 0.8 proxies and env variable set for 1.0.
The more I think about it - having the env set in the image seems better.
The proxy version and support tags are a property
of the proxy image used - not a global setting at install time (turned into
a global inject setting).
Costin
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6854 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAFI6lI45sb2_6MByyQcW0QgVTnQads0ks5uDqvbgaJpZM4VDJjP>
.
|
…xy_version_1.0
…into proxy_version_1.0
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
|
PR #6873 adds similar vars into the image - it doesn't hurt to have both.
IMHO the docker envs are better to provide info about the actual envoy that
is running - inject time options
are too inflexible, it's a global setting.
…On Thu, Jul 5, 2018 at 7:19 PM istio-bot ***@***.***> wrote:
[APPROVALNOTIFIER] This PR is *APPROVED*
This pull-request has been approved by: *costinm
<#6854 (comment)>*, *rshriram
<#6854#>*
The full list of commands accepted by this bot can be found here
<https://go.k8s.io/bot-commands>.
The pull request process is described here
<https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process>
Needs approval from an approver in each of these files:
- pilot/OWNERS
<https://github.com/istio/istio/blob/release-1.0/pilot/OWNERS>
[costinm,rshriram]
- pkg/OWNERS
<https://github.com/istio/istio/blob/release-1.0/pkg/OWNERS>
[costinm,rshriram]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6854 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAFI6gbx3blFkpu3OjCFTKNSi32yoaP2ks5uDsk1gaJpZM4VDJjP>
.
|
|
and I modified this PR 10 minutes ago to add env vars to the image :) |
|
No problem, I ended up adding a fix for tproxy mode in my PR, will change
the title...
…On Thu, Jul 5, 2018 at 7:30 PM Shriram Rajagopalan ***@***.***> wrote:
and I modified this PR 10 minutes ago to add env vars to the image :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6854 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAFI6nbCmc9szZSS9hQeDBuaCGPreepkks5uDsvhgaJpZM4VDJjP>
.
|
costinm
left a comment
There was a problem hiding this comment.
There is also proxy_debug - not sure who is using it ( the 300M one with not-optimized envoy), but fine to fix in in separate PR.
/lgtm
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: costinm, rshriram The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
To allow smooth upgrades from 1.0 to future versions.
I need help to determine the places where this needs to be plugged in.
cc @sdake @costinm @ayj
Signed-off-by: Shriram Rajagopalan shriramr@vmware.com