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

add dnspolicy #20

Merged
merged 4 commits into from
Sep 22, 2023
Merged

add dnspolicy #20

merged 4 commits into from
Sep 22, 2023

Conversation

maleck13
Copy link
Collaborator

@alexsnaps this PR moves the content over from the MGC repo to this one for DNSPolicy. Ok to merge?

Copy link
Member

@alexsnaps alexsnaps left a comment

Choose a reason for hiding this comment

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

Most of it (under ## Proposal feels like the # Guide-level explanation afaict?
Also, there @guicassolato had some comments on using a "more standard" label selector.

Anyways, on the merging, the idea is that RFC is full fledged proposal when merging. So since this is all still under "proposal", do we think that is (to the best of our current knowledge) the feature as we'd expose it to users? If so that'd cover the Guide-level explanation section... Reference-level explanation part of how it'd be implemented isn't really much spelled out, is it?

rfcs/0000-dns-policy.md Outdated Show resolved Hide resolved
- Feature Name: DNSPolicy
- Start Date: 2023-07-01
- RFC PR: [Kuadrant/architecture#0000](https://github.com/Kuadrant/architecture/pull/0000)
- Issue tracking: [Kuadrant/architecture#0000](https://github.com/Kuadrant/architecture/issues/0000)
Copy link
Member

Choose a reason for hiding this comment

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

This needs, before merging, a matching "tracking" issue. But thinking it might already exist?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated with the best issue for it

@maleck13
Copy link
Collaborator Author

Yes this has been updated as we have implemented the API so it should be as you say upto date for the guide level. How important is the reference level?
Often things get implemented in one way and then things change and get refactored etc so I worry this could get out of date rapidly. I could keep it at a high level IE

  • Add DNSPolicy CRD that conforms to policy attachment spec
  • Add DNSPolicy controller to MCG
  • DNS logic and record management should all migrate out of the gateway controller into this new controller as it is the responsibility and domain of the DNSPolicy controller to manage DNS

@maleck13
Copy link
Collaborator Author

Moved main body of text to guide-level
added a high level reference

@alexsnaps
Copy link
Member

@guicassolato wdyt? Merge as is?

@maleck13
Copy link
Collaborator Author

@guicassolato @alexsnaps as this a migration from the MGC repo wondering if we can merge at this point or is there something fundamental that needs to be addressed. IE we have already implemented it and documented it so I think any change would be a new RFC possibly?

Copy link
Member

@alexsnaps alexsnaps left a comment

Choose a reason for hiding this comment

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

Rename this to rfcs/0003-dns-policy.md and let's merge this.

@maleck13
Copy link
Collaborator Author

done

Copy link
Contributor

@guicassolato guicassolato left a comment

Choose a reason for hiding this comment

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

I think you forgot staging the renamed file, @maleck13

@maleck13
Copy link
Collaborator Author

maleck13 commented Sep 5, 2023

@alexsnaps @guicassolato woops :) fixed now

@maleck13
Copy link
Collaborator Author

@alexsnaps I have done all that was needed I think now, ok to merge?

@alexsnaps alexsnaps self-requested a review September 21, 2023 12:13
@maleck13
Copy link
Collaborator Author

@guicassolato I made the requested change

apiVersion: kuadrant.io/v1alpha1
kind: DNSPolicy
spec:
targetRef: # defaults to gateway gvk and current namespace
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@guicassolato guicassolato left a comment

Choose a reason for hiding this comment

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

Good base ref for the work already started around DNSPolicy.

Small comment left about a minor detail required to comply with GW-API PA types that, as of today, would not allow implementing exactly as exemplified, but it's certainly not the main focus of the RFC here, neither its impact on UX is of concern IMO.

I missed something about hostnames specified only at the level of the HTTPRoutes (subdomains), for which one may still want to set LB rules. My understanding is that, while limited to gateways, those would have to be brought up as additional listener definitions, whose limitations are known. I suppose this could be added in the Future Possibilities section or in a future iteration.

@maleck13
Copy link
Collaborator Author

Thanks @guicassolato yes I think section name support will be something we will look at in the near future

@maleck13 maleck13 merged commit faab728 into main Sep 22, 2023
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