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

network: add support for podman network update and --network-dns-server #16732

Merged
merged 1 commit into from
Jan 12, 2023
Merged

network: add support for podman network update and --network-dns-server #16732

merged 1 commit into from
Jan 12, 2023

Conversation

flouthoc
Copy link
Collaborator

@flouthoc flouthoc commented Dec 4, 2022 •
edited
Loading

  • Add support for podman network update <networkupdate>
network update

Description:
  update networks for containers and pods

Usage:
  podman network update [options] [NAME]

Examples:
  podman network update podman1

Options:
      --add-dns-servers stringArray      add network level nameservers
      --remove-dns-servers stringArray   remove network level nameservers
  • Add support for --network-dns-server to podman network create

Extends podman to support recently added features in netavark and aardvark-dns

[NO NEW TESTS NEEDED]
[NO TESTS NEEDED]

network: add support for `podman network update` and `--network-dns-server`

@flouthoc flouthoc marked this pull request as draft December 4, 2022 16:06
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 4, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc

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

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 4, 2022
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 10, 2022
@flouthoc flouthoc force-pushed the network-update branch 2 times, most recently from 919fa64 to d3bf94d Compare December 10, 2022 16:33
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 10, 2022
@flouthoc flouthoc force-pushed the network-update branch 4 times, most recently from a45ceb5 to f728618 Compare December 11, 2022 01:40
@flouthoc
Copy link
Collaborator Author

@cevich Podman CI is using older netavark and aardvark any idea how can i use https://github.com/containers/netavark/releases/tag/v1.4.0 and https://github.com/containers/aardvark-dns/releases/tag/v1.4.0 in CI

@flouthoc
Copy link
Collaborator Author

Other tests are failing because c/common is broken as same reason here: #16733

@github-actions github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Dec 11, 2022
@flouthoc flouthoc force-pushed the network-update branch 2 times, most recently from 4e324b8 to ceb5943 Compare December 11, 2022 10:26
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 13, 2022
@flouthoc flouthoc changed the title network: add support for podman network update and --network-dns-server [WIP] [CI:LATEST_NVAV] network: add support for podman network update and --network-dns-server Dec 13, 2022
@flouthoc
Copy link
Collaborator Author

flouthoc commented Dec 13, 2022 •
edited
Loading

Okay newly added tests pass with latest nv/av this PR will be ready for review once c/common issues are fixed are after #16733. cc @TomSweeneyRedHat for info.

@flouthoc
Copy link
Collaborator Author

@baude @Luap99 @mheon This PR is ready for review ( the test pass with latest netavark/aardvark ) but we have to wait for c/common fix and till podman CI starts using latest nv/av permanently.

@cevich
Copy link
Member

cevich commented Dec 14, 2022

till podman CI starts using latest nv/av permanently.

Since I'll be out on PTO for a few weeks, if you notice the packages have been released into Fedora updates-testing (F37) and updates (F36), please feel free to open up a c/automation_images PR. Most of the time it works, when it doesn't, it fails horribly and there's probably no hope so just give up 😁
(Lokesh knows a bit about how it works too, in case he comes back before me)

flouthoc
flouthoc commented Jan 11, 2023
View reviewed changes
test/e2e/run_networking_test.go
@@ -41,6 +43,56 @@ var _ = Describe("Podman run networking", func() {

})

It("podman verify network scoped DNS server and also verify updating network dns server", func() {
// TODO: Unskip after https://github.com/containers/podman/pull/16525
Skip("TODO: unskip after https://github.com/containers/podman/pull/16525")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As discussed this passes with latest nv/av but being skipped for now since CI does not contains latest nv/av.

@flouthoc flouthoc requested a review from Luap99 January 11, 2023 18:54
rhatdan
rhatdan reviewed Jan 11, 2023
View reviewed changes
cmd/podman/networks/update.go Outdated
if err != nil {
return err
}
fmt.Printf("%s", name)
Copy link
Member

Choose a reason for hiding this comment

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

Is this debug or intended?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is intentional we together decided that on network update success command must return network update , see: #16732 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

This should use Println(), it is missing a final newline sorry for not spotting this earlier.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay done.

TomSweeneyRedHat
TomSweeneyRedHat reviewed Jan 12, 2023
View reviewed changes
pkg/api/handlers/libpod/networks.go

networkUpdateOptions := entities.NetworkUpdateOptions{}
if err := json.NewDecoder(r.Body).Decode(&networkUpdateOptions); err != nil {
utils.Error(w, http.StatusBadRequest, fmt.Errorf("failed to decode request JSON payload: %w", err))
Copy link
Member

Choose a reason for hiding this comment

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

IDK, I'm on the fence. Maybe?

Suggested change
utils.Error(w, http.StatusBadRequest, fmt.Errorf("failed to decode request JSON payload: %w", err))
utils.Error(w, http.StatusBadRequest, fmt.Errorf("failed to decode JSON request payload: %w", err))

@flouthoc flouthoc force-pushed the network-update branch 4 times, most recently from 51c9eaa to bf4f71a Compare January 12, 2023 08:39
@rhatdan
Copy link
Member

rhatdan commented Jan 12, 2023

LGTM

@flouthoc
network: add support for podman network update and --network-dns-server
882cd17 
* Add support for `podman network update <>`

```console
network update

Description:
  update networks for containers and pods

Usage:
  podman network update [options] NAME

Examples:
  podman network update podman1

Options:
      --dns-add stringArray      add network level nameservers
      --dns-drop stringArray   remove network level nameservers
```

* Add support for `--network-dns-server` to `podman network create`

Extends podman to support recently added features in `netavark` and
`aardvark-dns`

* containers/netavark#497
* containers/aardvark-dns#252
* containers/netavark#503

[NO NEW TESTS NEEDED]
[NO TESTS NEEDED]

Signed-off-by: Aditya R <[email protected]>
@Luap99
Copy link
Member

Luap99 commented Jan 12, 2023

/lgtm
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 12, 2023
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 12, 2023
@flouthoc
Copy link
Collaborator Author

@Luap99 @TomSweeneyRedHat PTAL

@Luap99 Luap99 mentioned this pull request Jan 12, 2023
Closed
@Luap99
Copy link
Member

Luap99 commented Jan 12, 2023

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 12, 2023
@openshift-merge-robot openshift-merge-robot merged commit b107d77 into containers:main Jan 12, 2023
@edsantiago edsantiago mentioned this pull request Jan 12, 2023
Closed
@edsantiago edsantiago mentioned this pull request Jan 25, 2023
Closed
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 15, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@TomSweeneyRedHat TomSweeneyRedHat TomSweeneyRedHat left review comments

@rhatdan rhatdan rhatdan left review comments

@mheon mheon Awaiting requested review from mheon

@Luap99 Luap99 Awaiting requested review from Luap99

Assignees

@Luap99 Luap99

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/api-change Change to remote API; merits scrutiny lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@flouthoc @cevich @rhatdan @Luap99 @mheon @TomSweeneyRedHat @openshift-merge-robot