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

Handle existing zone in dynamically addition of DNS records #236

Merged
merged 1 commit into from
Jun 15, 2023

Conversation

balajiv113
Copy link
Contributor

In #217 i assumed that we are iterating through all zones until we find a record match but that was wrong. If the zone name matches we are returning error if record is not found.

This PR is about handling existing zones to keep the records array in sync.

Copy link
Contributor

@cfergeau cfergeau left a comment

Choose a reason for hiding this comment

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

The addition might need to be added slightly differently, see my comments in the PR.

pkg/services/dns/dns.go Outdated Show resolved Hide resolved
for i, zone := range s.handler.zones {
if zone.Name == req.Name {
req.Records = append(req.Records, zone.Records...)
s.handler.zones = append(s.handler.zones[:i], s.handler.zones[i+1:]...)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not so sure it's a good thing to reorder the zones list. Let's say the initial state is:

DNS: []types.Zone{
	{
		Name:      "crc.testing.",
		DefaultIP: net.ParseIP("192.168.127.2"),
	},
	{
		Name: "testing.",
		Records: []types.Record{
			{
				Name: "host",
				IP:   net.ParseIP("192.168.127.3"),
			},
		},
	},
},

And I then want to add gateway.testing:

types.Zone{
	{
		Name: "testing.",
		Records: []types.Record{
			{
				Name: "gateway",
				IP:   net.ParseIP("192.168.127.1"),
			},

	},

After updating the existing zone, testing. will come before crc.testing., and *.crc.testing will return "not found" instead of resolving to 192.168.127.2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah makes sense thanks for the input i missed out this usecase completely :( I will update the PR to maintain the order of zones as well.

Basically,

  • We will insert new zones at last (as we are already handling existing zones case)
  • For existing zones keep the location same. (Just append Records to existing one and set DefaultIP)

@balajiv113
Copy link
Contributor Author

@cfergeau
Addressed the use cases that you have mentioned. Also wrote some new unit test for verifying the addZone cases. Let me know if it looks good now.

Happy to update if any changes are required :)

@balajiv113 balajiv113 force-pushed the fix-existing-zone branch 2 times, most recently from 8105ff0 to d024333 Compare June 14, 2023 11:47
pkg/services/dns/dns.go Outdated Show resolved Hide resolved
pkg/services/dns/dns.go Outdated Show resolved Hide resolved
pkg/services/dns/dns.go Show resolved Hide resolved
Signed-off-by: Balaji Vijayakumar <kuttibalaji.v6@gmail.com>
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 15, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: balajiv113, cfergeau

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

@cfergeau cfergeau merged commit 3f1d1f3 into containers:main Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants