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.

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

libnetwork, Network: add field NetworkDNSServers for network scoped dns #1237

Merged

Conversation

flouthoc
Copy link
Collaborator

libnetwork must allow to pass network_dns_servers so aardvark and netavark can consume it and enabled network scoped dns.

Feature implemented at netavark and aardvark end

@flouthoc flouthoc requested a review from Luap99 November 21, 2022 05:55
@flouthoc
Copy link
Collaborator Author

@containers/podman-maintainers @Luap99 @mheon PTAL

@flouthoc flouthoc requested review from mheon and baude November 21, 2022 05:55
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

Since this field can only be set on network create you should validate it there.
a) make sure it only contains ips
b) it is only set when DNSEnabled is true
c) in the cni backend it should error out since we will not support it there.

And then don't forget to add tests for it.

@flouthoc
Copy link
Collaborator Author

flouthoc commented Nov 21, 2022

Since this field can only be set on network create you should validate it there. a) make sure it only contains ips b) it is only set when DNSEnabled is true c) in the cni backend it should error out since we will not support it there.

And then don't forget to add tests for it.

@Luap99 Did you mean validations in NetworkCreate API in libnetwork or the command podman network create we usually validate parsing details and types at frontend so should test be at the podman end ? I think we can add validation to both NetworkCreate at libnetwork and podman cli end.

@Luap99
Copy link
Member

Luap99 commented Nov 21, 2022

Validation should always be done in the backend in libnetwork.

@flouthoc
Copy link
Collaborator Author

@Luap99 SGTM.

One note though some of the validation and parse checks are being made at frontend like https://github.com/containers/podman/blob/main/cmd/podman/networks/create.go#L137 maybe we should move these to backend later on then for consistency.

@Luap99
Copy link
Member

Luap99 commented Nov 21, 2022

One note though some of the validation and parse checks are being made at frontend like https://github.com/containers/podman/blob/main/cmd/podman/networks/create.go#L137 maybe we should move these to backend later on then for consistency.

If you look at the Network format struct these checks only make sense in the frontend. It is impossible to set a gateway without subnet in the type so the cli has to validate the flags.

@flouthoc flouthoc force-pushed the network-scoped-dns-common branch from d022f31 to 6fe9da5 Compare November 21, 2022 15:42
@flouthoc flouthoc requested a review from Luap99 November 21, 2022 15:43
@flouthoc flouthoc force-pushed the network-scoped-dns-common branch from 6fe9da5 to 2089001 Compare November 21, 2022 15:45
libnetwork/netavark/config.go Outdated Show resolved Hide resolved
libnetwork/netavark/config.go Outdated Show resolved Hide resolved
libnetwork/netavark/config.go Outdated Show resolved Hide resolved
@flouthoc flouthoc force-pushed the network-scoped-dns-common branch from 2089001 to 72e6d51 Compare November 21, 2022 15:55
@flouthoc flouthoc requested a review from Luap99 November 21, 2022 15:56
libnetwork must allow to pass network_dns_servers so aardvark and
netavark can consume it and enabled network scoped dns.

Feature implemented at netavark and aardvark end
* Netavark: containers/netavark#497

Signed-off-by: Aditya R <arajan@redhat.com>
@flouthoc flouthoc force-pushed the network-scoped-dns-common branch from 72e6d51 to 0d903e3 Compare November 21, 2022 15:59
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM, but I guess we want the nv/av PRs to merge first?

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 21, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, Luap99

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@flouthoc
Copy link
Collaborator Author

@Luap99 Upto you and maintainers but I think this can go in as-is cause this PR is not blocked on nv/av. Podman and nv/av are just gonna use this.

@rhatdan
Copy link
Member

rhatdan commented Nov 21, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Nov 21, 2022
@openshift-merge-robot openshift-merge-robot merged commit f5d94c3 into containers:main Nov 21, 2022
@flouthoc
Copy link
Collaborator Author

Once Upstream nv/av PR gets merged I'll wire this into podman.

flouthoc added a commit to flouthoc/common that referenced this pull request Mar 10, 2023
We enforced NetworkDNSServers to be IP addresses and we follow this
enfore rule while a user is creating network, see comment
containers#1237 (review)
and PR containers#1237

Following check was missed in `NetworkUpdateOptions` hence add this
check now.

Signed-off-by: Aditya R <arajan@redhat.com>
flouthoc added a commit to flouthoc/common that referenced this pull request Mar 10, 2023
We enforced NetworkDNSServers to be IP addresses and we follow this
enfore rule while a user is creating network, see comment
containers#1237 (review)
and PR containers#1237

Following check was missed in `NetworkUpdateOptions` hence add this
check now.

Signed-off-by: Aditya R <arajan@redhat.com>
flouthoc added a commit to flouthoc/common that referenced this pull request Mar 30, 2023
We enforced NetworkDNSServers to be IP addresses and we follow this
enfore rule while a user is creating network, see comment
containers#1237 (review)
and PR containers#1237

Following check was missed in `NetworkUpdateOptions` hence add this
check now.

Backport of: containers#1358

Signed-off-by: Aditya R <arajan@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants