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.

Sign up for GitHub

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

Jump to bottom

Remove ManagedZone API #793

Merged
merged 4 commits into from
Aug 20, 2024
Merged

Remove ManagedZone API #793

merged 4 commits into from
Aug 20, 2024

Conversation

mikenairn
Copy link
Member

@mikenairn mikenairn commented Jul 31, 2024 •
edited
Loading

ToDo:

  • Review/Update quickstart
  • Review/Update all docs
  • Review/Update all tests

Requires:

@codecov Codecov
Copy link

codecov bot commented Jul 31, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 80.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 82.01%. Comparing base (ece13e8) to head (a30a6d6).
Report is 169 commits behind head on main.

Files Patch % Lines
api/v1alpha1/dnspolicy_types.go 70.00% 3 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #793      +/-   ##
==========================================
+ Coverage   80.20%   82.01%   +1.81%     
==========================================
  Files          64       76      +12     
  Lines        4492     6090    +1598     
==========================================
+ Hits         3603     4995    +1392     
- Misses        600      748     +148     
- Partials      289      347      +58
Flag Coverage Δ
bare-k8s-integration 4.50% <0.00%> (?)
controllers-integration 71.92% <80.00%> (?)
gatewayapi-integration 10.82% <0.00%> (?)
integration ?
istio-integration 56.26% <0.00%> (?)
unit 31.58% <0.00%> (+1.55%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
api/v1beta1 (u) 71.42% <ø> (ø)
api/v1beta2 (u) 85.35% <80.00%> (-6.08%) ⬇️
pkg/common (u) 88.13% <ø> (-0.70%) ⬇️
pkg/istio (u) 72.50% <ø> (-1.41%) ⬇️
pkg/log (u) 94.73% <ø> (ø)
pkg/reconcilers (u) ∅ <ø> (∅)
pkg/rlptools (u) 83.64% <ø> (+4.19%) ⬆️
controllers (i) 82.36% <82.22%> (+5.56%) ⬆️
Files Coverage Δ
controllers/dns_helper.go 90.37% <ø> (+6.39%) ⬆️
controllers/dnspolicy_controller.go 79.01% <ø> (-4.47%) ⬇️
controllers/dnspolicy_dnsrecords.go 62.03% <100.00%> (-3.65%) ⬇️
api/v1alpha1/dnspolicy_types.go 74.75% <70.00%> (-7.07%) ⬇️

... and 39 files with indirect coverage changes

@mikenairn mikenairn force-pushed the remove_managed_zone_api branch 3 times, most recently from 6a9758a to 790496c Compare August 1, 2024 14:44
@mikenairn mikenairn force-pushed the remove_managed_zone_api branch 5 times, most recently from a84a90c to ea3f66e Compare August 14, 2024 09:14
@mikenairn mikenairn changed the title WIP chore: Remove ManagedZone API Remove ManagedZone API Aug 14, 2024
@mikenairn mikenairn mentioned this pull request Aug 14, 2024
Merged
maleck13
maleck13 reviewed Aug 19, 2024
View reviewed changes
doc/user-guides/secure-protect-connect.md Outdated

```sh
export AWS_ACCESS_KEY_ID=xxxxxxx # Key ID from AWS with Route 53 access
export AWS_SECRET_ACCESS_KEY=xxxxxxx # Access key from AWS with Route 53 access
export AWS_DNS_PUBLIC_ZONE_ID=xxxxxx # DNS Zone ID in AWS Route 53
Copy link
Collaborator

Choose a reason for hiding this comment

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

will remove

@maleck13 maleck13 marked this pull request as ready for review August 19, 2024 11:22
@maleck13 maleck13 force-pushed the remove_managed_zone_api branch 5 times, most recently from c9e3aaf to 0c01ad9 Compare August 19, 2024 13:41
@mikenairn @maleck13
chore: Remove ManagedZone API
bc61465 
Signed-off-by: Michael Nairn <[email protected]>

Signed-off-by: craig <[email protected]>
@maleck13 maleck13 force-pushed the remove_managed_zone_api branch from bab2b4c to bc61465 Compare August 20, 2024 07:29
eguzki
eguzki reviewed Aug 20, 2024
View reviewed changes
Copy link
Contributor

@eguzki eguzki left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. One question before giving LGTM, though.

examples/dnspolicy/aws-dns-provider-secret.yaml
stringData:
AWS_ACCESS_KEY_ID: ${KUADRANT_AWS_ACCESS_KEY_ID}
AWS_SECRET_ACCESS_KEY: ${KUADRANT_AWS_SECRET_ACCESS_KEY}
AWS_REGION: ${KUADRANT_AWS_REGION}
Copy link
Contributor

Choose a reason for hiding this comment

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

missing ZONE_ID_FILTER and ZONE_DOMAIN_FILTER ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

These are no longer there. Instead we rely on the credential to filter down via permissions. Rather than having it in two places, suggestion is to create a secret with a credential that has access to the zones you want kuadrant to manage

Copy link
Collaborator

Choose a reason for hiding this comment

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

going to do a quick check for any other refs to that

Copy link
Collaborator

Choose a reason for hiding this comment

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

note now those are not needed, we will search the available zones for a matching zone and assign it to the dnsrecords. If there are 2 zones that match we will error out

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I am confused. Do you mean they are optional or should not be there?

I see those two fields being added multiple times:

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. I noticed these also. Sorry took this PR over so was unsure. They are gone. So I will remove from those docs also

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok should all be gone now @eguzki I also added an example AWS policy to set permissions

azgabur
azgabur reviewed Aug 20, 2024
View reviewed changes
Copy link

@azgabur azgabur left a comment

Choose a reason for hiding this comment

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

doc/reference/dnspolicy.md needs updating as well.

The information about the base domain name that was previously in ManagedZone spec.domainName key is now lost or it can be mentioned in the dns Provider Secret somehow?

@maleck13
Copy link
Collaborator

@eguzki @azgabur should be updated now with the changes

@maleck13 maleck13 mentioned this pull request Aug 20, 2024
Closed
9 tasks
azgabur
azgabur previously requested changes Aug 20, 2024
View reviewed changes
Copy link

@azgabur azgabur left a comment

Choose a reason for hiding this comment

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

doc/reference/dnspolicy.md is still not updated with the mention of providerRefs key

@maleck13
Copy link
Collaborator

@azgabur my bad sorry. I updated a different doc. I see what you are referring to now 👍

@eguzki
Copy link
Contributor

eguzki commented Aug 20, 2024

LGTM

didierofrivia
didierofrivia approved these changes Aug 20, 2024
View reviewed changes
Copy link
Member

@didierofrivia didierofrivia left a comment

Choose a reason for hiding this comment

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

🧑‍🍳

@maleck13 maleck13 merged commit 12687cb into main Aug 20, 2024
29 checks passed
@azgabur azgabur mentioned this pull request Aug 21, 2024
Closed
4 tasks
@mikenairn mikenairn mentioned this pull request Aug 27, 2024
Merged
@smccarthy-ie smccarthy-ie mentioned this pull request Sep 3, 2024
Open
@mikenairn mikenairn deleted the remove_managed_zone_api branch September 30, 2024 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eguzki eguzki eguzki left review comments

@maleck13 maleck13 maleck13 left review comments

@didierofrivia didierofrivia didierofrivia approved these changes

@azgabur azgabur azgabur left review comments

Assignees
No one assigned
Labels
None yet
Projects
Kuadrant
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mikenairn @maleck13 @eguzki @didierofrivia @azgabur