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

Only create one delegation set #947

Merged
merged 5 commits into from
Jan 9, 2025
Merged

Only create one delegation set #947

merged 5 commits into from
Jan 9, 2025

Conversation

lionello
Copy link
Member

@lionello lionello commented Jan 8, 2025

Description

The fix is to not keep creating new delegation sets, but only create when necessary. There's two situations:

  • Hosted zone already exists. In this case we try to create the delegation set from the zone, which fails if it's "already reusable", in which case we get it from the zone directly;
  • No hosted zone. We find an existing delegation set, and create one if none was found. Note that on subsequent deployments this will revert to the first case, ie. hosted zone already exists and we'll end up with the same delegation set that we created in the first place!

I've added tests that work both as unit tests (using R53 mock) and integration test!

Linked Issues

Fixes #946

Checklist

  • I have performed a self-review of my code
  • I have added appropriate tests
  • I have updated the Defang CLI docs and/or README to reflect my changes, if necessary

@lionello lionello requested a review from nullfunc January 8, 2025 23:21
src/pkg/cli/client/byoc/aws/byoc.go Show resolved Hide resolved
src/pkg/cli/client/byoc/aws/byoc.go Outdated Show resolved Hide resolved
src/pkg/cli/client/byoc/aws/byoc.go Outdated Show resolved Hide resolved
src/pkg/cli/client/byoc/aws/byoc.go Outdated Show resolved Hide resolved
src/pkg/cli/client/byoc/aws/byoc.go Outdated Show resolved Hide resolved
src/pkg/cli/client/byoc/aws/byoc.go Outdated Show resolved Hide resolved
src/pkg/cli/client/byoc/aws/byoc.go Outdated Show resolved Hide resolved
src/pkg/cli/client/byoc/aws/byoc.go Outdated Show resolved Hide resolved
@lionello lionello changed the title Only create one delegations set Only create one delegation set Jan 9, 2025
Copy link
Member

@jordanstephens jordanstephens left a comment

Choose a reason for hiding this comment

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

Thank you for the refactor, it's much easier to follow! I still have some questions about how this works, but it seems reasonable given my limited understanding.

src/pkg/cli/client/byoc/aws/domain.go Outdated Show resolved Hide resolved
src/pkg/cli/client/byoc/aws/domain.go Show resolved Hide resolved
src/pkg/cli/client/byoc/aws/domain.go Show resolved Hide resolved
src/pkg/cli/client/byoc/aws/domain.go Show resolved Hide resolved
@lionello lionello merged commit 6a70473 into main Jan 9, 2025
12 checks passed
@lionello lionello deleted the lio/reuse-delegation-set branch January 9, 2025 21:34
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.

Fix Route53 delegation set quota exceeded
3 participants