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

Make selection of subnet using docker more flexible. #557

Merged
merged 2 commits into from
Apr 22, 2024

Conversation

Boomatang
Copy link
Contributor

This removes the assumption that the first item in IPAM.Config is the IPv4 entry. However the assumption that only the IPv4 has a value for the gateway is being made.

@Boomatang Boomatang requested a review from a team as a code owner April 19, 2024 13:59
@Boomatang Boomatang added kind/bug Something isn't working size/small labels Apr 19, 2024
@eguzki
Copy link
Contributor

eguzki commented Apr 22, 2024

We had

## Parse kind network subnet
## Take only IPv4 subnets, exclude IPv6
SUBNET=$(docker network inspect $networkName --format '{{json .IPAM.Config }}' | \
    ${YQ} '.[] | select( .Subnet | test("^((25[0-5]|(2[0-4]|1\d|[1-9]|)\d)\.?\b){4}/\d+$")) | .Subnet')
if [[ -z "$SUBNET" ]]; then
   echo "Error: parsing IPv4 network address for '$networkName' docker network"
   exit 1
fi

in the first place and it was changed on #517 when adding podman local setup. Maybe @david-martin has a good reason.

Maybe add it back?

@eguzki
Copy link
Contributor

eguzki commented Apr 22, 2024

When I run

docker network inspect kind | yq e '.[] | select(.IPAM.Config[] | has("Gateway")) | .IPAM.Config[] | select(has("Gateway")) | .Subnet' -

I get

172.18.0.0/16
fc00:f853:ccd:e793::/64

When I run

docker network inspect kind --format '{{json .IPAM.Config }}' | \
    yq '.[] | select( .Subnet | test("^((25[0-5]|(2[0-4]|1\d|[1-9]|)\d)\.?\b){4}/\d+$")) | .Subnet'

I get

172.18.0.0/16

@Boomatang
Copy link
Contributor Author

I'll update this to include the regex. My assumption was there would be no gateway on the IPv6, which in my case was true. Gladly will fix.

@david-martin
Copy link
Contributor

in the first place and it was changed on #517 when adding podman local setup. Maybe @david-martin has a good reason

The change landed on a working branch here before the changes landed back on main.
@jasonmadigan do you recall the reason for the change to the network inspect cmd? I think the suggested cmd came from the kind docs or metallb docs?

Either way, reverting the docker version of the cmd sounds reasonable if it fixes an issue and still gives the desired behaviour.

@jasonmadigan
Copy link
Member

@jasonmadigan do you recall the reason for the change to the network inspect cmd? I think the suggested cmd came from the kind docs or metallb docs?

Either way, reverting the docker version of the cmd sounds reasonable if it fixes an issue and still gives the desired behaviour.

I don't recall, but this change/revert (with the regex) looks fine to me. No longer have docker so someone else should probably test.

@Boomatang Boomatang merged commit 11840b8 into Kuadrant:main Apr 22, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working size/small
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants