[Galley] Adding ServiceEntry synthesis#11293
[Galley] Adding ServiceEntry synthesis#11293nmittler wants to merge 1 commit intoistio:release-1.1from
Conversation
da57306 to
c8df2ff
Compare
c8df2ff to
7ae150c
Compare
There was a problem hiding this comment.
Is it worth using the k8s defined constants for these labels?
There was a problem hiding this comment.
It might be nice for these constants to be accessible by Pilot so we don't need to redefine them (e.g. see here).
There was a problem hiding this comment.
as discussed offline ... I'll remove the custom collection here in favor of just reusing the existing ServiceEntry collection. Pilot will know that these are special since: 1) it will have to create a client that listens on a different group (not "default") and 2) We'll add a special annotation to indicate that the ServiceEntry is synthetic.
There was a problem hiding this comment.
And duplicating the constants is less of a problem than introducing a dependency.
7ae150c to
a8e195a
Compare
sdake
left a comment
There was a problem hiding this comment.
@nmittler I understand this PR is WIP. Looks pretty good already! How do you plan to enable this for the operator? ENV variable, config flag? How do you enable this PR in your local env (e.g. if I wanted to eval the PR, how would I?)
Cheers
-steve
a8e195a to
f7d4dd7
Compare
Currently, the new flow will be enabled at the source end. I've previously introduced a new flag that filters creation of sources for certain types. By default, this new functionality is "off", since the default filter excludes Pods, Nodes, Endpoints, and Services. |
d7eba74 to
5808d0a
Compare
|
is this okay to merge? Would like some ack from galley team |
|
Looked at this before the last update with benchmarks, lgtm. I'll take a look again for the recent changes. As for getting this in, I don't think this is part of the PR short list @duderino is keeping for 1.1 though. |
galley/pkg/runtime/projections/serviceentry/convert/convert_bench_test.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Minor nit: if you use t.Run(...) equivalent of b here, then you don't need to reset timer. It should also account memory allocations only within the context of the lambda.
There was a problem hiding this comment.
I'm just not sure what the test name should be in that case. I'd rather err on the side of a clear test name here.
There was a problem hiding this comment.
Can you make these data driven? It looks like there is an opportunity to set the right data model here for adding more variations of the test.
There was a problem hiding this comment.
we can revisit once we have a second data set.
galley/pkg/runtime/projections/serviceentry/convert/convert_bench_test.go
Outdated
Show resolved
Hide resolved
galley/pkg/runtime/projections/serviceentry/convert/convert_bench_test.go
Outdated
Show resolved
Hide resolved
pkg/spiffe/spiffe.go
Outdated
There was a problem hiding this comment.
Not a problem with your checkin in particular, but do the ns and serviceAccount need to be escaped?
There was a problem hiding this comment.
I'm not sure.
@quanjielin @wattli may have more
|
@ozevren PTAL ... added one more commit, which adds an integration-level benchmark |
galley/pkg/runtime/projections/serviceentry/integration/integration_bench_test.go
Outdated
Show resolved
Hide resolved
galley/pkg/runtime/projections/serviceentry/integration/integration_bench_test.go
Outdated
Show resolved
Hide resolved
galley/pkg/runtime/projections/serviceentry/integration/integration_bench_test.go
Outdated
Show resolved
Hide resolved
93593bc to
7344080
Compare
|
/test e2e-mixer-no_auth |
galley/pkg/runtime/projections/serviceentry/integration/integration_bench_test.go
Outdated
Show resolved
Hide resolved
galley/pkg/runtime/projections/serviceentry/integration/integration_bench_test.go
Outdated
Show resolved
Hide resolved
d594205 to
8ef20f3
Compare
There was a problem hiding this comment.
Can we just simply set the debug log level as part of init and not worry about resetting it? Would make the test code cleaner.
There was a problem hiding this comment.
I considered that, but I'd rather not interfere with other tests.
0fa500e to
367cac5
Compare
|
/test istio-pilot-e2e-envoyv2-v1alpha3 |
Added a new custom projection that is subscribed to events for k8s Pods, Nodes, Services and Endpoints. These events are absorbed and do not become part of the snapshot. Instead, synthetic ServiceEntry resources are generated and become part of the snapshot. Partially addresses istio#10497 and istio#10589
|
@nmittler: The following tests failed, say
DetailsInstructions 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. |
|
The new file synthetic/serviceEntry contains in fact k8s nodes, pods, services and it is under networking/v1alpha3. I was expecting that directory to contain only Istio APIs and serviceEntry.yaml to contain maybe some examples of generated ServiceEntry. The KRMs should be somewhere else, maybe in the file-system adapter? |
It doesn't really matter much where these files go. I chose to put it under v1alpha3 because it generates a v1alpha3 proto. AFAIK there's not really a pattern established where input and output are from different packages.
Again, I don't entirely agree .... and it doesn't really matter much. |
|
It may not matter, but it is confusing, so please rename the file from serviceEntry.yaml to something else like krm.yaml (Kubernetes Resource Model). The directory networking/v1alpha3 should also contain examples for user-defined ServiceEntries, those belong in a file called serviceEntry.yaml. Please add tests for user-defined serviceEntries for external services (it can be separate PR). |
|
Closing this in favor of #12409 (against the master branch) |
Added a new custom projection that is subscribed to events for k8s Pods, Nodes, Services and Endpoints. These events are absorbed and do not become part of the snapshot. Instead, synthetic ServiceEntry resources are generated and become part of the snapshot.
Partially addresses #10497 and #10589