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.

Sign up for GitHub

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

Jump to bottom

Add DNS capture into CNI. #32416

Merged
merged 10 commits into from
May 4, 2021
Merged

Add DNS capture into CNI. #32416

merged 10 commits into from
May 4, 2021

Conversation

bianpengyuan
Copy link
Contributor

@bianpengyuan bianpengyuan commented Apr 23, 2021 •
edited
Loading

fix #29511

@istio-testing istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Apr 23, 2021
@istio-testing
Copy link
Collaborator

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@google-cla google-cla bot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Apr 23, 2021
@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 23, 2021
howardjohn
howardjohn reviewed Apr 23, 2021
View reviewed changes
Copy link
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

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

Looks great

cni/cmd/istio-cni/main.go Outdated Show resolved Hide resolved
cni/cmd/istio-cni/redirect.go Outdated Show resolved Hide resolved
@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Apr 25, 2021
@istio-testing istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Apr 25, 2021
@bianpengyuan
Copy link
Contributor Author

@howardjohn Do you think if it is ok to make istio cni enablement a test framework flag? Local DNS cache test fails with CNI which we know won't work: https://prow.istio.io/view/gs/istio-prow/pr-logs/pull/istio_istio/32416/integ-pilot-k8s-tests_istio/1387096174833438720. We should just skip those cases when CNI is enabled, but that information is tricky to get without istio.enable_cni being a test framework flag.

howardjohn
howardjohn reviewed Apr 27, 2021
View reviewed changes
Copy link
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

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

Its a more general problem really. We are starting to treat our tests more and more like "conformance tests" where we bring some environment and then run a bunch of tests against it. So we have things like RequireIstioVersion, RequireClusterVersion, --istio.test.revision, --istio.test.skipVM, etc. We are also making tests workload agnostic which is pretty similar

We probably want a more general solution long term where a test can declare its dependencies and a test-runner can declare how its setup.

Short term a flag is probably fine, or in the job that runs CNI just add --istio.test.skip?

@stevenctl for more opinions

@stevenctl
Copy link
Contributor

I think a flag is fine; maybe we need some general flag like --istio.test.features=-vm,+cni where we skip tests that require VMs or skip tests that will break with CNI enabled.

If we do add --istio.test.cniEnabled, I'm not sure we should have it control setting the install options, or if the flag should just help inform test skips.

@bianpengyuan
Copy link
Contributor Author

Ok, thanks for the feedback. I will add a flag for now then.

If we do add --istio.test.cniEnabled, I'm not sure we should have it control setting the install options, or if the flag should just help inform test skips.

I think we should just control all CNI related stuff with this flag if we have it. Long term, when we have a feature skipping flag, we can go back and use operator install options. I created an issue to track this: #32493

@bianpengyuan
Copy link
Contributor Author

/test all

@bianpengyuan bianpengyuan marked this pull request as ready for review April 28, 2021 03:36
@bianpengyuan bianpengyuan requested review from a team as code owners April 28, 2021 03:36
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Apr 28, 2021
@bianpengyuan
Copy link
Contributor Author

@howardjohn @stevenctl ptal thanks!

@istio-testing istio-testing merged commit 22efe35 into istio:master May 4, 2021
@neerajaustin
Copy link

Hello, I am waiting on this fix to be used in our product deployment. Please indicate which release version would this be available? I see istio 1.10 is released. So just wanted to get an idea.

@jbilliau-rcd
Copy link

Same here; we are waiting to implement the new SmartDNS feature until this is released; currently on 1.8.6

@bianpengyuan
Copy link
Contributor Author

Unfortunately this did not meet 1.10 cut. Will be in 1.11.

@diranged
Copy link
Contributor

Unfortunately this did not meet 1.10 cut. Will be in 1.11.

Quick question - any chance we can either run a pre-release in our dev clusters to try it out, or you can shed some light on when 1.11 is scheduled for release?

@howardjohn
Copy link
Member

howardjohn commented Jun 15, 2021 via email

@diranged
Copy link
Contributor

@howardjohn thanks i can confirm that on the 1.11-dev release, the istio-cni is properly setting up the DNS iptables rules.. thanks!!

@noah8713
Copy link
Contributor

@howardjohn @bianpengyuan Can we also backport this to 1.10 too?

shonecyx pushed a commit to shonecyx/istio that referenced this pull request Dec 24, 2021
@bianpengyuan @noah8713
Add DNS capture into CNI. (istio#32416)
7593d6d 
* Add DNS capture into CNI.

* fix.

* update

* fix

* capture all

* fix.

* Add a flag for CNI enablement in integration test.

* remove cni test args.
@SheilaghM SheilaghM mentioned this pull request Jan 15, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@howardjohn howardjohn howardjohn approved these changes

Assignees
No one assigned
Labels
cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DNS redirect iptables rules not created by istio-cni
8 participants
@bianpengyuan @istio-testing @stevenctl @neerajaustin @jbilliau-rcd @diranged @howardjohn @noah8713