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

Remove limit of one IP address per A record, modify Task.IPs behavior… #553

Merged
merged 2 commits into from
Aug 19, 2020

Conversation

drewkerrigan
Copy link
Contributor

… to return only IPs from first populated source

Addresses #541

  • Added second network_infos to a task in factories/fake.json
  • Added corresponding test in records/generator_test.go
  • Removed ipsTo4And6 in records/generator.go, it was unnecessarily limiting IP lists to a single ipv4 and ipv6 address for no good reason other than to perhaps avoid creating A records for more than one source
  • Modified behavior of Task.IPs in records/state/state.go such that it returns all ip addresses from the first populated configured source
    • The previous behavior was to return all available IP addresses from all configured sources. These additional IPs were never used, because the list was always limited to a single ipv4 and ipv6 address.
  • Updated a test in records/state/state_test.go to pass with the new Task.IPs behavior

Drew Kerrigan added 2 commits July 29, 2020 11:38
@drewkerrigan
Copy link
Contributor Author

To test the changes, I've uploaded binaries here: https://github.com/drewkerrigan/mesos-dns/releases/tag/v0.7.1

@f1-outsourcing
Copy link

Drew I will ask support to have a look at this.

"ip_address": "12.0.1.3"
}
],
"name": "dcos6"
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically not a problem, but lulz. ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The network name? That's actually the real default ipv6 network name in dcos :-) https://github.com/dcos/dcos/blob/270cb17ec173dc52e69892c1c3ba796414f46936/gen/calc.py#L1140

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, but that's not an ipv6 address ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, missed that!

@jkoelker jkoelker self-assigned this Aug 17, 2020
@jkoelker
Copy link
Contributor

Howdy! Thanks for taking care of this. It LGTM, but as I'm not 100% on the ramifications on other parts of DC/OS I asked internally for peeps to let me know by 2300 UTC on Wednesday if they see an issue with multiple addresses. From a DNS/Networking POV, it totes makes sense, but just want to make sure there aren't issues that we'll need to shake out elsewhere before incorporating it. If I dont' hear anything back I'll merge it on Wednesday. Happy Hacking!

@f1-outsourcing
Copy link

Howdy! Thanks for taking care of this. It LGTM, but as I'm not 100% on the ramifications on other parts of DC/OS I asked internally for peeps to let me know by 2300 UTC on Wednesday if they see an issue with multiple addresses. From a DNS/Networking POV, it totes makes sense, but just want to make sure there aren't issues that we'll need to shake out elsewhere before incorporating it. If I dont' hear anything back I'll merge it on Wednesday. Happy Hacking!

This is an issue[1] I have noticed, but since (I guess) most dcos users do not seem to use multiple ip's, this should not be much of an issue.

[1]
#554

@jkoelker jkoelker merged commit e888ef4 into mesosphere:master Aug 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants