-
Notifications
You must be signed in to change notification settings - Fork 194
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
CORS-3907: Update ingress operator to with custom endpoints #1197
base: master
Are you sure you want to change the base?
Conversation
@barbacbd: This pull request references CORS-3907 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
/jira refresh |
@barbacbd: This pull request references CORS-3907 which is a valid jira issue. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enhancement link for more context: Enhancement link for more context: openshift/enhancements#1734
@barbacbd might be worth adding to the description
Also worth noting that this is the equivalent of the existing AWS implementation here:
cluster-ingress-operator/pkg/dns/aws/dns.go
Lines 209 to 243 in 69cadab
tagFound := false | |
for _, ep := range config.ServiceEndpoints { | |
// TODO: Add custom endpoint support for elbv2. See the following for details: | |
// https://docs.aws.amazon.com/general/latest/gr/elb.html | |
switch ep.Name { | |
case Route53Service: | |
route53Found = true | |
r53Config = r53Config.WithEndpoint(ep.URL) | |
log.Info("Found route53 custom endpoint", "url", ep.URL) | |
case TaggingService: | |
if tagConfig == nil { | |
log.Info(fmt.Sprintf("Found resourcegroupstaggingapi custom endpoint, which will be ignored since the %s region does not support that API", region)) | |
continue | |
} | |
tagFound = true | |
url := ep.URL | |
// route53 for govcloud is based out of us-gov-west-1, | |
// so the tagging client must match. | |
if strings.Contains(ep.URL, "us-gov-east-1") { | |
url = govCloudTaggingEndpoint | |
} | |
tagConfig = tagConfig.WithEndpoint(url) | |
log.Info("Found resourcegroupstaggingapi custom endpoint", "url", url) | |
case ELBService: | |
elbFound = true | |
elbConfig = elbConfig.WithEndpoint(ep.URL) | |
log.Info("Found elb custom endpoint", "url", ep.URL) | |
} | |
// Once the three service endpoints have been found, | |
// ignore any further service endpoint specifications. | |
if route53Found && elbFound && tagFound { | |
break | |
} | |
} | |
} |
For AWS there is an LB and tagging client as well, but not sure why. Those should be handled by cloud-provider-aws AFAIK
This looks simple enough I feel comfortable to LGTM.
pkg/dns/gcp/provider.go
Outdated
for _, endpoint := range config.Endpoints { | ||
if endpoint.Name == configv1.GCPServiceEndpointNameDNS { | ||
options = append(options, option.WithEndpoint(endpoint.URL)) | ||
break | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially you could wrap this for loop with the feature gate... I think that would be technically correct
The installer will only lay down the config if the feature gate is enabled, so it is already effectively gated. I would defer to the preferences of the owning team.
@barbacbd: This pull request references CORS-3907 which is a valid jira issue. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
/retest-required |
The tagging client is used to look up the hosted zone by tags, and the elb client is used to look up the target hosted zone from the ELB. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@patrickdillon, @barbacbd, since the GCP custom endpoints feature involves multiple components but is being implemented by the Installer team developers, is the Installer team QE going to verify the changes in cluster-ingress-operator, or is that going to be entirely NI&D QE's responsibility?
@@ -114,6 +114,7 @@ type Config struct { | |||
// PrivateHostedZoneAWSEnabled indicates whether the "SharedVPC" feature gate is | |||
// enabled. | |||
PrivateHostedZoneAWSEnabled bool | |||
CustomEndpointsEnabled bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new field should have "GCP" in the name in order to make it obvious that this is GCP-specific, and to avoid a potential conflict if we eventually add a similar featuregate for Azure (the other platforms already have APIs for specifying custom service endpoints, so it looks like Azure is the only potential future conflict). The field should also have godoc.
CustomEndpointsEnabled bool | |
// CustomEndpointsEnabled indicates whether the "GCPCustomAPIEndpoints" | |
// featuregate is enabled. | |
CustomEndpointsEnabled bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Miciah I made those changes. One of the tests I believe is still failing so I need to look into that though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI During QE's installation testing of the feature, we will ensure that all the cluster operators, including the ingress operator, are healthy finally.
** If an endpoint exists for the DNS service use the endpoint as an option when creating the new client.
/retest-required |
/retest |
@barbacbd: The following tests failed, say
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-sigs/prow repository. I understand the commands that are listed here. |
Enhancement link for the feature: openshift/enhancements#1734
** Vendor update
** Set the endpoint information in the Config for GCP
** If an endpoint exists for the DNS service use the endpoint as an option when creating the new client.