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

Shrink NS names #490

Merged
merged 1 commit into from
May 20, 2021
Merged

Shrink NS names #490

merged 1 commit into from
May 20, 2021

Conversation

kuritka
Copy link
Collaborator

@kuritka kuritka commented May 17, 2021

closes #456

nsServerName and nsExtServerNames - shortly, we calculate nsnames from ClusterGeoTag and ExtClusterGeoTags.

  • DNS_ZONE k8gb-test.gslb.cloud.example.com
  • EDGE_DNS_ZONE: cloud.example.com
  • CLUSTER_GEOTAG: us
    will generate gslb-ns-us-k8gb-test-gslb.cloud.example.com

Nsnames are provided by depresolver config via functions (c *Config) GetExternalClusterNSNames() (m map[string]string) and (c *Config) GetClusterNSName() string and are validated during k8gb initialization. Regarding RFC1035, particular DNS labels are validated on max length 63 characters and whole domain is validated on max length 253 characters [1]. We cover that by unit tests.

Because nsnames are provided via config, nsServerName(...) and nsServerNameExt(...) were removed including tests.
Because nsname structure had changed I had to fix controller_tests, infoblox_tests, values.yaml and Makefile.

I also moved to configuration GetExternalClusterHeartbeatFQDNs(gslbName string) (m map[string]string) and GetClusterHeartbeatFQDN(gslbName string) string which are used in parallel with GetExternalClusterNSNames() and are also calculated from the configuration data.

For testing I'm using gomock, which creates mock objects and asserts intermediate results. Thanks to that I'm able to assert endpoint before we save it, see TestCreateZoneDelegationOnExternalDNS in external_test.go. GoMock generates mock files based on the interface. Each make mocks will regenerate mock files without license. That's why golic runs for each *_mock.go.

Signed-off-by: kuritka kuritka@gmail.com

@kuritka kuritka force-pushed the 456-RFC1035-63chars-size-limit-3 branch 5 times, most recently from 9e8fb85 to 43af203 Compare May 19, 2021 07:57
Copy link
Member

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

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

Just a minor inline enhancement, otherwise looks great!

controllers/depresolver/depresolver_test.go Outdated Show resolved Hide resolved
closes #456

`nsServerName` and `nsExtServerNames` - shortly nsnames are calculated from ClusterGeoTag and ExtClusterGeoTags.

- DNS_ZONE k8gb-test.gslb.cloud.example.com
- EDGE_DNS_ZONE: cloud.example.com
- CLUSTER_GEOTAG: us
 will generate **gslb-ns-us-k8gb-test-gslb.cloud.example.com**

Nsnames are provided by depresolver config via functions `(c *Config) GetExternalClusterNSNames() (m map[string]string)` and `(c *Config) GetClusterNSName() string` and
are validated during k8gb initialization. Regarding [RFC1035](https://www.ietf.org/rfc/rfc1035.txt), particular
DNS labels are validated on max length 63 characters and whole domain is validated on max length 253 characters
[1](https://stackoverflow.com/questions/32290167/what-is-the-maximum-length-of-a-dns-name). Such behavior is covered by unit tests.

Because nsnames are provided via config, `nsServerName(...)` and `nsServerNameExt(...)` were removed including tests.
Because nsname structure had changed I had to fix `controller_tests`, `infoblox_tests`, `values.yaml` and `Makefile`.

I also moved to configuration `GetExternalClusterHeartbeatFQDNs(gslbName string) (m map[string]string)` and `GetClusterHeartbeatFQDN(gslbName string) string`
which are used in parallel with `GetExternalClusterNSNames()` and are also calculated from the configuration data.

Signed-off-by: kuritka <kuritka@gmail.com>
@kuritka kuritka force-pushed the 456-RFC1035-63chars-size-limit-3 branch from 43af203 to 0f4042e Compare May 19, 2021 09:11
@kuritka kuritka requested a review from ytsarev May 19, 2021 09:16
Copy link
Member

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@somaritane somaritane left a comment

Choose a reason for hiding this comment

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

LGTM

@kuritka kuritka merged commit 2702e9e into master May 20, 2021
@kuritka kuritka deleted the 456-RFC1035-63chars-size-limit-3 branch May 20, 2021 09:35
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.

Shorten NS names for zone delegation
3 participants