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

rfc for removal of managed zone API #93

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

maleck13
Copy link
Collaborator

@maleck13 maleck13 commented Jun 27, 2024

Copy link

@pehala pehala left a comment

Choose a reason for hiding this comment

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

My biggest problem with this RFC is that it is bundling together two things that are not neccessarily connected in my eyes:

  1. Removal of the ManagedZoned and replacing the content of credential secret
  2. Adding ability to have multiple credentials for DNSPolicy

rfcs/0011-removal-managed-zone.md Outdated Show resolved Hide resolved
@maleck13
Copy link
Collaborator Author

maleck13 commented Jul 2, 2024

My biggest problem with this RFC is that it is bundling together two things that are not neccessarily connected in my eyes:

  1. Removal of the ManagedZoned and replacing the content of credential secret
  2. Adding ability to have multiple credentials for DNSPolicy

Sure I can see that. But we have to remove the managedzone ref as part of this work and replace it with a secretRef anyway, so it seemed a reasonable to question if we should go with a single secret or allow multiple as part of this work allowing us to remove a limitation the DNSPolicy has currently and avoid two breaking changes one after the other. So this was really an attempt to be somewhat efficient and remove the need for another RFC.

@pehala
Copy link

pehala commented Jul 2, 2024

Sure I can see that. But we have to remove the managedzone ref as part of this work and replace it with a secretRef anyway, so it seemed a reasonable to question if we should go with a single secret or allow multiple as part of this work allowing us to remove a limitation the DNSPolicy has currently and avoid two breaking changes one after the other. So this was really an attempt to be somewhat efficient and remove the need for another RFC.

Sure, my problem with this is that while removing managedzone is simple enough and probably will go through without much debate. Multiple credentials is a new feature and might require more discussion. When I looked through the PRs I didnt pay much attention to this RFC because I thought it handles just the removal.

I would either split this into two RFC or rescope this RFC to focus mainly on the new feature and as part of that we will remove ManagedZone

@maleck13
Copy link
Collaborator Author

maleck13 commented Jul 2, 2024

Sure I can see that. But we have to remove the managedzone ref as part of this work and replace it with a secretRef anyway, so it seemed a reasonable to question if we should go with a single secret or allow multiple as part of this work allowing us to remove a limitation the DNSPolicy has currently and avoid two breaking changes one after the other. So this was really an attempt to be somewhat efficient and remove the need for another RFC.

Sure, my problem with this is that while removing managedzone is simple enough and probably will go through without much debate. Multiple credentials is a new feature and might require more discussion. When I looked through the PRs I didnt pay much attention to this RFC because I thought it handles just the removal.

I would either split this into two RFC or rescope this RFC to focus mainly on the new feature and as part of that we will remove ManagedZone

Yeah I think that is reasonable. And to be fair the concept emerged as part of thinking about removal. So I guess we can say the original RFC was intended to just be removal of the MZ but our thinking evolved and so re-focusing the RFC to be around supporting multiple providers and domains from a single policy is a reasonable approach or removing that part into a separate RFC 👍

@maleck13
Copy link
Collaborator Author

@mikenairn could I get a review on this

```

Validation of the secret structure will happen in the dns operator rather than doing it once in the kuadrant operator and again in the dns operator. If the secret doesn't exist or if the required fields are not present the status of the DNSRecord will be updated to reflect the issue and the DNSPOlicy will reflect this status condition in its existing `recordConditions` section as it does now.
DNSPolicy controller will only validate that the specified DOMAIN exists in the target gateway. If it does not exist it will mark this in the status of DNSPolicy.
Copy link
Member

@mikenairn mikenairn Jul 12, 2024

Choose a reason for hiding this comment

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

What does this mean? Is the policy controller having to read data from the providerRef(secret)?

```

ZONE_ID: somezone
DOMAIN_NAME: *.a.b.com
Copy link
Member

@mikenairn mikenairn Jul 12, 2024

Choose a reason for hiding this comment

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

The only reason i see needing the DOMAIN_NAME in the secret would be if we intend to look it up, much like we do currently for ManagedZones, is that the intention? Are we still only creating a DNSRecord if we have a providerRef that has a domain set that matches a listener host? If that is the case, domain name here would not have the wildcard on it, it should match the zone domain name a.b.com.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes I forgot to remove this after our conversation online... Will update. I think we just create the DNSRecord and allow the DNS Opertor to do the work and pass back the result.

@alexsnaps alexsnaps added the RFC Request For Comments label Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC Request For Comments
Projects
Status: Ready For Review
Development

Successfully merging this pull request may close these issues.

RFC: Remove Managed Zone from DNSPolicy
6 participants