-
Notifications
You must be signed in to change notification settings - Fork 33
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
dnspolicy section name support #961
Conversation
4790f37
to
910cb75
Compare
Changes looks very simple. +1 |
910cb75
to
52af9ea
Compare
52af9ea
to
995e546
Compare
Signed-off-by: Michael Nairn <[email protected]>
DNSRecords are currently owned by the DNSPolicy that created them. We can't just update the ownership on policy change since the new policy may not be compatible with the current DNSRecord, instead we must re-create the DNSRecord resource. This is not ideal, issue to look into changing this Kuadrant/dns-operator#287 Signed-off-by: Michael Nairn <[email protected]>
4311467
to
1818be2
Compare
Returns true if an existing record can knowingly be updated to a desired state based on the differences of the specs. Current known reasons for not being able to update are: * RootHost updates * Endpoint record type changes (A -> CNAME etc..) In both these cases, and any others that may be added to `canUpdateDNSRecord` the current record should be deleted before doing a create of the desired record. Signed-off-by: Michael Nairn <[email protected]>
1818be2
to
504d443
Compare
Verifiation
👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
} | ||
|
||
return true | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maleck13 fyi, because i had to do this to deal with possible changes in record type when attaching a policy to a listener that already has existing records via policy attached to a gateway, it means we could probably remove this now https://github.com/Kuadrant/kuadrant-operator/blob/main/api/v1alpha1/dnspolicy_types.go#L56 and allow switching from simple to loadbalanced without the need for manual intervention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice ok. Lets go for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do it in a different PR, will need some tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add section name support to DNSPolicy.