fqdn: add fqdn proxy interface #17318

nebril · 2021-09-06T11:24:57Z

This change allows us to delete some of the test-specific code in agent and adds an interface for FQDN proxy.

nebril · 2021-09-06T11:27:24Z

test-me-please

Job 'Cilium-PR-K8s-1.16-net-next' has 3 failures but they might be new flakes since it also hit 1 known flakes: #17060 (83.27)

nebril · 2021-09-07T10:48:24Z

test-me-please

Job 'Cilium-PR-Runtime-net-next' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

RuntimePrivilegedUnitTests Run Tests

Failure Output

FAIL: Failed to run privileged unit tests

If it is a flake, comment /mlh new-flake Cilium-PR-Runtime-net-next so I can create a new GitHub issue to track it.

jibi · 2021-09-07T12:17:53Z

pkg/fqdn/proxy/proxy.go

+)
+
+type FQDNProxy interface {
+	GetRules(uint16) (restore.DNSRules, error)


Why does this need to return also an error given that both the DNSProxy and the MockFQDNProxy implementations cannot fail?

FWIW the DNSProxy implementation does have error conditions, we just don't hand them back up the stack at the moment. Ultimately I think that the current error condition should not be blocking the implementation of DNS policy so it doesn't make a huge difference where we handle that error, but technically it could be up to the parent functions to handle those errors appropriately (for instance, by outputting them to the endpoint-specific log rather than spamming the global log like the current implementation does).

nebril · 2021-09-07T16:13:48Z

test-me-please

Job 'Cilium-PR-Runtime-net-next' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

RuntimePrivilegedUnitTests Run Tests

Failure Output

FAIL: Failed to run privileged unit tests

If it is a flake, comment /mlh new-flake Cilium-PR-Runtime-net-next so I can create a new GitHub issue to track it.

Job 'Cilium-PR-K8s-1.16-net-next' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sPolicyTest Multi-node policy test with L7 policy using connectivity-check to check datapath

Failure Output

FAIL: cannot install connectivity-check

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.16-net-next so I can create a new GitHub issue to track it.

nebril · 2021-09-08T07:44:36Z

test-me-please

pkg/fqdn/proxy/proxy.go

joestringer · 2021-09-13T22:49:41Z

pkg/fqdn/proxy/proxy.go

+)
+
+type FQDNProxy interface {
+	GetRules(uint16) (restore.DNSRules, error)


FWIW the DNSProxy implementation does have error conditions, we just don't hand them back up the stack at the moment. Ultimately I think that the current error condition should not be blocking the implementation of DNS policy so it doesn't make a huge difference where we handle that error, but technically it could be up to the parent functions to handle those errors appropriately (for instance, by outputting them to the endpoint-specific log rather than spamming the global log like the current implementation does).

jrajahalme

LGTM, but consider the interface name change proposed by Joe.

pkg/fqdn/proxy/proxy.go

nebril · 2021-09-16T11:05:08Z

test-me-please

Job 'Cilium-PR-K8s-GKE' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: Test pods are not ready after timeout

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-GKE so I can create a new GitHub issue to track it.

This change adds interface for abstracting away FQDN proxy Signed-off-by: Maciej Kwiek <maciej@isovalent.com>

This change allows for daemon integration tests to run with mock DNS proxy Signed-off-by: Maciej Kwiek <maciej@isovalent.com>

nebril · 2021-09-22T12:59:10Z

test-me-please

nebril · 2021-09-24T12:12:16Z

Hit #16938 in both conformance kind and eks tests

nebril requested a review from a team September 6, 2021 11:24

nebril requested review from a team as code owners September 6, 2021 11:24

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Sep 6, 2021

nebril requested review from jibi and jrajahalme September 6, 2021 11:25

maintainer-s-little-helper bot assigned jibi and jrajahalme Sep 6, 2021

nebril force-pushed the pr/nebril/fqdn-proxy-interface branch from 8454020 to acd7c5a Compare September 6, 2021 11:27

maintainer-s-little-helper bot mentioned this pull request Sep 6, 2021

CI: K8sServicesTest Checks service across nodes Tests NodePort BPF Tests with vxlan Test NodePort with netfilterCompatMode=true #17060

Closed

nebril force-pushed the pr/nebril/fqdn-proxy-interface branch from acd7c5a to f217162 Compare September 7, 2021 10:48

jibi reviewed Sep 7, 2021

View reviewed changes

nebril force-pushed the pr/nebril/fqdn-proxy-interface branch from f217162 to 0ee8a1c Compare September 7, 2021 16:13

nebril force-pushed the pr/nebril/fqdn-proxy-interface branch from 0ee8a1c to 95d1297 Compare September 8, 2021 07:44

nebril added the release-note/misc This PR makes changes that have no direct user impact. label Sep 8, 2021

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Sep 8, 2021

joestringer reviewed Sep 13, 2021

View reviewed changes

jrajahalme approved these changes Sep 14, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned jrajahalme Sep 14, 2021

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 14, 2021

christarazi added dont-merge/bad-bot To prevent MLH from marking ready-to-merge. and removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Sep 16, 2021

christarazi requested a review from jibi September 16, 2021 07:12

christarazi requested changes Sep 16, 2021

View reviewed changes

pkg/fqdn/proxy/proxy.go Outdated Show resolved Hide resolved

nebril force-pushed the pr/nebril/fqdn-proxy-interface branch from 95d1297 to 4c9955d Compare September 16, 2021 11:04

jibi approved these changes Sep 16, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned jibi Sep 16, 2021

christarazi approved these changes Sep 16, 2021

View reviewed changes

nebril added 2 commits September 22, 2021 14:57

fqdn: Add proxy interface

2278f2d

This change adds interface for abstracting away FQDN proxy Signed-off-by: Maciej Kwiek <maciej@isovalent.com>

daemon: add mock dns proxy to tests

b0876c6

This change allows for daemon integration tests to run with mock DNS proxy Signed-off-by: Maciej Kwiek <maciej@isovalent.com>

nebril force-pushed the pr/nebril/fqdn-proxy-interface branch from 4c9955d to b0876c6 Compare September 22, 2021 12:57

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 28, 2021

nebril merged commit 0666f53 into cilium:master Sep 28, 2021

nebril deleted the pr/nebril/fqdn-proxy-interface branch September 28, 2021 11:28

nebril added the needs-backport/1.10 label Oct 11, 2021

maintainer-s-little-helper bot added this to Needs backport from master in 1.10.5 Oct 11, 2021

errordeveloper mentioned this pull request Oct 12, 2021

v1.10 backports 2021-10-12 #17582

Merged

errordeveloper added backport-pending/1.10 and removed needs-backport/1.10 labels Oct 12, 2021

maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.10.5 Oct 12, 2021

joestringer added backport-done/1.10 and removed backport-pending/1.10 labels Oct 13, 2021

maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.5 Oct 13, 2021

joestringer mentioned this pull request Oct 13, 2021

Prepare for release v1.10.5 #17606

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fqdn: add fqdn proxy interface #17318

fqdn: add fqdn proxy interface #17318

nebril commented Sep 6, 2021

nebril commented Sep 6, 2021 •

edited by maintainer-s-little-helper bot

nebril commented Sep 7, 2021 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

jibi Sep 7, 2021

joestringer Sep 13, 2021

nebril commented Sep 7, 2021 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

Test Name

Failure Output

nebril commented Sep 8, 2021

joestringer Sep 13, 2021

jrajahalme left a comment

nebril commented Sep 16, 2021 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

nebril commented Sep 22, 2021

nebril commented Sep 24, 2021

fqdn: add fqdn proxy interface #17318

fqdn: add fqdn proxy interface #17318

Conversation

nebril commented Sep 6, 2021

nebril commented Sep 6, 2021 • edited by maintainer-s-little-helper bot

nebril commented Sep 7, 2021 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

jibi Sep 7, 2021

Choose a reason for hiding this comment

joestringer Sep 13, 2021

Choose a reason for hiding this comment

nebril commented Sep 7, 2021 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

Test Name

Failure Output

nebril commented Sep 8, 2021

joestringer Sep 13, 2021

Choose a reason for hiding this comment

jrajahalme left a comment

Choose a reason for hiding this comment

nebril commented Sep 16, 2021 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

nebril commented Sep 22, 2021

nebril commented Sep 24, 2021

nebril commented Sep 6, 2021 •

edited by maintainer-s-little-helper bot

nebril commented Sep 7, 2021 •

edited by maintainer-s-little-helper bot

nebril commented Sep 7, 2021 •

edited by maintainer-s-little-helper bot

nebril commented Sep 16, 2021 •

edited by maintainer-s-little-helper bot