Add source of DNS configuration to AWSPlatformStatus #1397

sadasu · 2023-02-03T20:28:06Z

When and AWS customer wishes to use their own DNS solution instead of Route53, they are expected to create the API and API-Int Load Balancers and use their LB names to configure their custom DNS prior to cluster installation. The customer created API and API-Int LB information is provided via install-config.

AWSPlatformStatus is updated with a special "provider" if the user does not want OpenShift to configure the default cloud DNS solution.

Enhancement Proposal : openshift/enhancements#1400

openshift-ci · 2023-02-03T20:28:15Z

Hello @sadasu! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

For merging purposes, this repository follows the no-Feature-Freeze process which means that in addition to the standard lgtm and approved labels this repository requires either:

bugzilla/valid-bug - applied if your PR references a valid bugzilla bug

OR

qe-approved, docs-approved, and px-approved - these labels can be applied by anyone in the openshift org via the /label <labelname> command.

Who should apply these qe/docs/px labels?

For a no-Feature-Freeze team who is merging a feature before code freeze, they need to get those labels applied to their api repo PR by the appropriate teams (i.e. qe, docs, px)
For a Feature Freeze (traditional) team who is merging a feature before FF, they can self-apply the labels (via /label commands), they are basically irrelevant for those teams
For a Feature Freeze team who is merging a feature after FF, the PR should be rejected barring an exception

config/v1/types_infrastructure.go

JoelSpeed · 2023-03-06T17:42:20Z

config/v1/types_infrastructure.go

+
+// DNSConfig contains the LB IP and DNS Record type to configure a DNS entry.
+type DNSConfig struct {
+	// recordType is the DNS record type.


Are there a subset that we support here? Would we expect an MX for example? Could this be an enum of the ones we support?

Yes, it would be useful to make this an enum type. Updating.

config/v1/types_infrastructure.go

JoelSpeed · 2023-03-10T14:00:02Z

Heads up, I'm on PTO for the next 2 weeks, so if you need additional reviews in that time, I'd recommend poking in #forum-api-review

openshift-ci · 2023-06-29T01:29:43Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sadasu
Once this PR has been reviewed and has the lgtm label, please assign bparees for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

patrickdillon

Left some more comments for discussion. There are still quite a few references to load balancers in the code comments.

patrickdillon · 2023-07-18T15:30:47Z

config/v1/types_infrastructure.go

+	// +default={"provider": "ClusterProvided"}
+	// +kubebuilder:default={"provider": "ClusterProvided"}
+	// +openshift:enable:FeatureSets=TechPreviewNoUpgrade
+	DNSConfig *DNSConfigurationType `json:"dnsConfig"`


This should be optional, right? Upgraded clusters won't have this value.

Kubebuilder will automatically add the default value of provider="ClusterProvided". There is a integration test to that effect.

But, updating anyways.

Kubebuilder will automatically add the default value of provider="ClusterProvided". There is a integration test to that effect.

Interesting. I didn't know you could do that! That's cool.

I'm having a hard time wrapping my head around the idea of something being both required and having a default value; to me they seem mutually exclusive, or at least to mutually diminish one another. Just a personal observation/opinion.

Joel helpfullly brought up UPI installs--we don't technically know that all existing clusters are using IPI-style cloud DNS. Users could opt for UPI in order to establish their own non-route53 DNS solutions. I don't know how many that would be, but I'm pretty confident it's non-zero. So if we default this value, I think the values would likely be incorrect in some cases.

With the current implementation, if DNSConfig.Provider is left unset by the user, it will be given the default of "CloudProvided". "CloudProvided" represents how the cluster behaves today (without the custom DNS feature.) Afaik, this should work for UPI too. Are there some functionality/semantics issues that I am missing?

I replaced "CloudProvided" with "Default" to indicate that that the right defaults for UPI or IPI are in use.

Replaced "Default" with "".

config/v1/types_infrastructure.go

patrickdillon · 2023-07-18T16:02:43Z

config/v1/types_infrastructure.go

+	// DNSUserProvided indicates that the user provides the LB and DNS for API and API-Int.
+	DNSUserProvided DNSProviderType = "UserProvided"
+
+	// DNSClusterProvided indicates that the cluster provides the LB and DNS for API and API-Int.
+	DNSClusterProvided DNSProviderType = "ClusterProvided"


These values seem a little squishy to me. I am having trouble keeping track of all the discussions around this feature, so I'm not sure if these values have been gamed out.

My understanding of the intention here is that when the value is UserProvided that would indicate that we need to run CoreDNS. That to me has some cognitive dissonance.

For the sake of discussion, what about something along the lines of
CloudProvider - this is standard IPI, DNS is in the cloud provider
ClusterProvider - uses on-cluster baremetal dns provider (coredns)
UserProvider - cluster/ipi does not provide DNS, this would be for something like ROSA where they are running dnsmasq in pods

Do these values better capture the current/future use cases we are aware of?

Because in a way, the cluster is taking care of DNS for both the CloudProvider and ClusterProvider cases. I have updated the values for Provider as UserProvided and CloudProvided. Comments have been updated accordingly.

I see the cluster's need to run the CoreDNS pod for Ingress as an implementation detail. Agree that there have been several discussions on this topic. The enhancement proposal reflects the current approach we are taking.

patrickdillon · 2023-07-18T16:05:45Z

config/v1/types_infrastructure.go

+// DNSConfigurationType contains information about API and API-Int Load Balancers
+// and who is responsible for that configuration.
+// +union
+type DNSConfigurationType struct {


To enable external DNS, I think we are only concerned about using coreDNS to run ingress. Should that be taken into account for the shape of this API? Any concerns there?

I think this comment was unclear. My concern is that this shape may give the impression that this is the DNS config for everything (API & Ingress) but I believe our current direction is that this will only treat ingress.

The comment suggests you are looking at this config as "who provides DNS for Ingress?" Whereas, this API is currently designed to indicate "who provides DNS for the cluster?". I think the 2nd question is the most generic form of the question.

We need DNS for Ingress to be functional during cluster bringup so we have a CoreDNS pod that provides that during cluster bring up. When the cluster is up, we do want the user to update their DNS solution for the *.apps entry too. When all steps are completed, the customer's DNS solution resolves API, API-Int and Ingress.

The enhancement proposal openshift/enhancements#1400 talks about this in greater detail.

config/v1/types_infrastructure.go

When a customer configures their own DNS solution for API and API-Int in place of the default cloud provided DNS, the state of the DNS provider is added to the Infrastructure CR's AWSPlatformStatus. This feature is in TechPreview.

Update the MaxLength within the format package of gomega. This allows for the test suite to compare strings of size greater than 4000 which for required for the new tests added for the custom DNS feature.

Currently feature is in Tech preview

sadasu · 2023-07-25T17:14:08Z

@JoelSpeed @patrickdillon could you please take another look?

patrickdillon · 2023-07-25T18:01:52Z

@JoelSpeed @patrickdillon could you please take another look?

Our enhancement discussion has revealed a path that may very well make these changes unneeded, so we should probably resolve that before iterating much further.

sadasu · 2023-07-25T20:42:35Z

@JoelSpeed @patrickdillon could you please take another look?

Our enhancement discussion has revealed a path that may very well make these changes unneeded, so we should probably resolve that before iterating much further.

@patrickdillon is there a desire for having the cluster have specific knowledge about whether external DNS is in use? If yes, then we would need this.

openshift-ci · 2023-08-31T10:47:54Z

@sadasu: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/verify-crd-schema	`3011226`	link	true	`/test verify-crd-schema`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-merge-robot · 2023-08-31T10:48:00Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

sadasu · 2023-10-04T18:24:41Z

This PR is no longer needed since the design for this epic has changed.

sadasu mentioned this pull request Feb 3, 2023

CORS-2323: Allow users to bring their own custom DNS solution for API and API-Int openshift/installer#6829

Closed

openshift-ci bot requested review from deads2k and sttts February 3, 2023 20:28

sadasu force-pushed the custom-dns branch from cccdcca to d024c79 Compare February 3, 2023 21:58

sadasu changed the title ~~Add DNS config for API, API-Int and Ingress to AWSPlatformStatus~~ WIP: Add DNS config for API, API-Int and Ingress to AWSPlatformStatus Feb 4, 2023

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 4, 2023

sadasu force-pushed the custom-dns branch 4 times, most recently from bccf116 to c1fa499 Compare February 10, 2023 14:41

sadasu changed the title ~~WIP: Add DNS config for API, API-Int and Ingress to AWSPlatformStatus~~ Add DNS config for API, API-Int and Ingress to AWSPlatformStatus Feb 10, 2023

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 10, 2023

sadasu force-pushed the custom-dns branch 4 times, most recently from 5285fc0 to b96d90f Compare February 14, 2023 19:43

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 18, 2023

sadasu force-pushed the custom-dns branch from b96d90f to cf9b979 Compare February 21, 2023 16:52

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 21, 2023

sadasu mentioned this pull request Mar 3, 2023

Add support for custom DNS on AWS openshift/enhancements#1276

Closed

JoelSpeed reviewed Mar 6, 2023

View reviewed changes

sadasu changed the title ~~Add DNS config for API, API-Int and Ingress to AWSPlatformStatus~~ Add API and API-Int LB information to AWSPlatformStatus when DNS is also pe-configured Jun 29, 2023

openshift-ci bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 29, 2023

sadasu force-pushed the custom-dns branch from cf9b979 to f5e0200 Compare June 29, 2023 01:29

openshift-ci bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 29, 2023

sadasu changed the title ~~Add API and API-Int LB information to AWSPlatformStatus when DNS is also pe-configured~~ Add API and API-Int LB information to AWSPlatformStatus when DNS pre-configured Jun 29, 2023

sadasu force-pushed the custom-dns branch from 1f91153 to 871c4da Compare July 18, 2023 14:59

patrickdillon reviewed Jul 18, 2023

View reviewed changes

sadasu force-pushed the custom-dns branch from 871c4da to 728176f Compare July 18, 2023 17:22

sadasu mentioned this pull request Jul 18, 2023

CORS-2062: Allow Customers to BYO DNS openshift/enhancements#1400

Closed

sadasu changed the title ~~Add API and API-Int LB information to AWSPlatformStatus when DNS pre-configured~~ Add source of DNS configuration to AWSPlatformStatus Jul 18, 2023

sadasu commented Jul 18, 2023

View reviewed changes

config/v1/types_infrastructure.go Outdated Show resolved Hide resolved

sadasu force-pushed the custom-dns branch 3 times, most recently from 567239c to d9303e4 Compare July 20, 2023 20:14

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 20, 2023

sadasu force-pushed the custom-dns branch from d9303e4 to eff20f7 Compare July 20, 2023 21:36

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 20, 2023

openshift-ci bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 20, 2023

sadasu force-pushed the custom-dns branch from eff20f7 to 651d169 Compare July 20, 2023 23:52

openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 20, 2023

sadasu force-pushed the custom-dns branch from 651d169 to 00c7cac Compare July 21, 2023 18:04

sadasu added 3 commits July 21, 2023 16:24

Add custom DNS configuration to AWSPlatformStatus

615aa69

When a customer configures their own DNS solution for API and API-Int in place of the default cloud provided DNS, the state of the DNS provider is added to the Infrastructure CR's AWSPlatformStatus. This feature is in TechPreview.

Allow testsuite to compare strings of length > 4000

4d307a7

Update the MaxLength within the format package of gomega. This allows for the test suite to compare strings of size greater than 4000 which for required for the new tests added for the custom DNS feature.

Add new tests to verify behavior of the new byo DNS feature

3011226

Currently feature is in Tech preview

sadasu force-pushed the custom-dns branch from 00c7cac to 3011226 Compare July 21, 2023 20:25

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 31, 2023

sadasu closed this Oct 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add source of DNS configuration to AWSPlatformStatus #1397

Add source of DNS configuration to AWSPlatformStatus #1397

sadasu commented Feb 3, 2023 •

edited

Loading

openshift-ci bot commented Feb 3, 2023

JoelSpeed Mar 6, 2023

sadasu Mar 9, 2023

JoelSpeed commented Mar 10, 2023

openshift-ci bot commented Jun 29, 2023

patrickdillon left a comment

patrickdillon Jul 18, 2023

sadasu Jul 18, 2023

sadasu Jul 18, 2023

patrickdillon Jul 19, 2023

sadasu Jul 19, 2023 •

edited

Loading

sadasu Jul 20, 2023

sadasu Jul 21, 2023

patrickdillon Jul 18, 2023

sadasu Jul 18, 2023 •

edited

Loading

patrickdillon Jul 18, 2023

patrickdillon Jul 18, 2023

sadasu Jul 18, 2023 •

edited

Loading

sadasu commented Jul 25, 2023

patrickdillon commented Jul 25, 2023

sadasu commented Jul 25, 2023

openshift-ci bot commented Aug 31, 2023

openshift-merge-robot commented Aug 31, 2023

sadasu commented Oct 4, 2023

Add source of DNS configuration to AWSPlatformStatus #1397

Add source of DNS configuration to AWSPlatformStatus #1397

Conversation

sadasu commented Feb 3, 2023 • edited Loading

openshift-ci bot commented Feb 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoelSpeed commented Mar 10, 2023

openshift-ci bot commented Jun 29, 2023

patrickdillon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadasu Jul 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadasu Jul 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadasu Jul 18, 2023 • edited Loading

Choose a reason for hiding this comment

sadasu commented Jul 25, 2023

patrickdillon commented Jul 25, 2023

sadasu commented Jul 25, 2023

openshift-ci bot commented Aug 31, 2023

openshift-merge-robot commented Aug 31, 2023

sadasu commented Oct 4, 2023

sadasu commented Feb 3, 2023 •

edited

Loading

sadasu Jul 19, 2023 •

edited

Loading

sadasu Jul 18, 2023 •

edited

Loading

sadasu Jul 18, 2023 •

edited

Loading