DNS: Add awsPrivateHostedZoneRole by patrickdillon · Pull Request #1460 · openshift/api

patrickdillon · 2023-05-16T14:42:09Z

Adds a field to the DNS config to hold a role to assume when performing cross- account installs in an AWS shared VPC
environment.

openshift/enhancements#1397

openshift-ci · 2023-05-16T14:42:27Z

Hello @patrickdillon! 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.

patrickdillon · 2023-05-16T14:47:59Z

cc @mrunalp @Miciah @gcs278 @mtulio

patrickdillon · 2023-05-16T14:49:43Z

We are still deciding between whether this should live in cluster infrastructure or dns. Currently this only has the DNS implementation, which I believe is preferred.

If it's helpful, I can also add the infrastructure implementation in a separate commit. Please LMK if that would be helpful and I can add it.

config/v1/types_dns.go

mrunalp · 2023-05-22T16:21:37Z

@deads2k ptal.

config/v1/types_dns.go

deads2k

a few additional changes please.

config/v1/types_dns.go

patrickdillon · 2023-05-24T02:34:17Z

config/v1/types_dns.go

+// +kubebuilder:validation:XValidation:rule="has(self.platform) && self.platform == 'AWS' ?  has(self.aws) : !has(self.aws)",message="aws configuration is required when platform is AWS, and forbidden otherwise"
+type DNSPlatformSpec struct {
+	// type is the underlying infrastructure provider for the cluster.
+	// Allowed values: "AWS".


@deads2k suggested allowed values be AWS and "", but that seems to contrast with the validation suggested here, which is what I went with.

The intent being: dns.spec.platform is optional, but if it is provided dns.spec.platform.type is required

Empty needs to be allowed to handle upgrade cases.

I have handled this in the code, but we couldn't figure out a test case (which you mentioned in slack). None of the fields in this config are mutable (although I'm not certain that is enforced).

Was trying a test like this:

onUpdate: - name: Should allow empty platform on upgrade initial: | apiVersion: config.openshift.io/v1 kind: DNS spec: zone: id: foo updated: | apiVersion: config.openshift.io/v1 kind: DNS spec: zone: id: bar expected: | apiVersion: config.openshift.io/v1 kind: DNS spec: zone: id: bar platform: type: ""

But that is failing

[FAILED] the following fields were expected to match but did not:
[(spec.platform/spec.platform) (spec.zone/spec.zone)]

We're not really sure how to write the upgrade test

patrickdillon · 2023-05-24T02:37:09Z

@ingvagabund and @deads2k thanks for the review. I have addressed the feedback (except I still need to add the integration tests), but have a few resulting questions. Thanks!

config/v1/techpreview.dns.testsuite.yaml

patrickdillon · 2023-05-26T19:22:17Z

@ingvagabund @deads2k I think all feedback has been addressed (except one question about tests)

deads2k · 2023-05-30T19:09:27Z

/approve
/assign @Miciah

Miciah · 2023-05-30T19:12:49Z

config/v1/techpreview.dns.testsuite.yaml

+  - name: Should not be able to specify different type and platform
+    initial: |
+      apiVersion: config.openshift.io/v1
+      kind: DNS
+      spec:
+        platform:
+          type: ""
+          aws:
+            privateZoneIAMRole: arn:aws:iam:123456789012:role/foo
+    expectedError: "Invalid value: \"object\": aws configuration is required when platform is AWS, and forbidden otherwise"
+  - name: Should not be able to specify different type and platform
+    initial: |
+      apiVersion: config.openshift.io/v1
+      kind: DNS
+      spec:
+        platform:
+          type: ""
+          aws:
+            privateZoneIAMRole: arn:aws:iam:123456789012:role/foo
+    expectedError: "Invalid value: \"object\": aws configuration is required when platform is AWS, and forbidden otherwise"


You have the same test case twice.

config/v1/types_dns.go

Miciah · 2023-05-30T19:19:24Z

config/v1/0000_10_config-operator_01_dns-TechPreviewNoUpgrade.crd.yaml

+                      enum:
+                        - ""
+                        - AWS
+                        - Azure
+                        - BareMetal
+                        - GCP
+                        - Libvirt
+                        - OpenStack
+                        - None
+                        - VSphere
+                        - oVirt
+                        - IBMCloud
+                        - KubeVirt
+                        - EquinixMetal
+                        - PowerVS
+                        - AlibabaCloud
+                        - Nutanix
+                        - External


Is this going to cause confusing error messages, or does the x-kubernetes-validations rule take precedence and prevent the enum rule from causing unsupported values to be printed in error messages?

In the test, the x-kubernetes-validations message takes precedence, so I think we're ok.

Adds a field to the DNS config to hold a role to assume when performing cross- account installs in an AWS shared VPC environment. Co-authored-by: Grant Spence <gcs278@vt.edu> Co-authored-by: deads2k

openshift-ci · 2023-05-30T20:07:52Z

@patrickdillon: all tests passed!

Full PR test history. Your PR dashboard.

Details

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.

Miciah · 2023-05-30T20:09:59Z

/lgtm

openshift-ci · 2023-05-30T20:11:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, Miciah, patrickdillon

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [deads2k]

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

openshift-ci bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 16, 2023

openshift-ci bot requested review from adambkaplan and knobunc May 16, 2023 14:42

gcs278 reviewed May 17, 2023

View reviewed changes

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

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

gcs278 reviewed May 17, 2023

View reviewed changes

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

patrickdillon commented May 22, 2023

View reviewed changes

config/v1/types_dns.go Show resolved Hide resolved

patrickdillon force-pushed the aws-shared-vpc branch from c28314e to 7c74afe Compare May 22, 2023 14:41

patrickdillon force-pushed the aws-shared-vpc branch from 7c74afe to 0345ef8 Compare May 23, 2023 00:03

ingvagabund reviewed May 23, 2023

View reviewed changes

config/v1/types_dns.go Show resolved Hide resolved

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

deads2k requested changes May 23, 2023

View reviewed changes

config/v1/types_dns.go Show resolved Hide resolved

config/v1/types_dns.go Show resolved Hide resolved

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

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

patrickdillon commented May 24, 2023

View reviewed changes

config/v1/types_dns.go Show resolved Hide resolved

patrickdillon commented May 24, 2023

View reviewed changes

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

patrickdillon commented May 24, 2023

View reviewed changes

patrickdillon force-pushed the aws-shared-vpc branch 2 times, most recently from 731d048 to b68af6b Compare May 26, 2023 18:56

gcs278 reviewed May 26, 2023

View reviewed changes

config/v1/techpreview.dns.testsuite.yaml Outdated Show resolved Hide resolved

gcs278 reviewed May 26, 2023

View reviewed changes

config/v1/techpreview.dns.testsuite.yaml Outdated Show resolved Hide resolved

patrickdillon force-pushed the aws-shared-vpc branch from b68af6b to 55be8ed Compare May 26, 2023 19:11

patrickdillon force-pushed the aws-shared-vpc branch from 55be8ed to 597c4d9 Compare May 30, 2023 19:02

openshift-ci bot assigned Miciah May 30, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 30, 2023

Miciah reviewed May 30, 2023

View reviewed changes

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

Miciah reviewed May 30, 2023

View reviewed changes

patrickdillon force-pushed the aws-shared-vpc branch from 597c4d9 to c49c061 Compare May 30, 2023 19:26

DNS: Add awsPrivateHostedZoneRole

815017c

Adds a field to the DNS config to hold a role to assume when performing cross- account installs in an AWS shared VPC environment. Co-authored-by: Grant Spence <gcs278@vt.edu> Co-authored-by: deads2k

patrickdillon force-pushed the aws-shared-vpc branch from c49c061 to 815017c Compare May 30, 2023 19:50

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 30, 2023

openshift-merge-robot merged commit 83abb00 into openshift:master May 30, 2023

JoelSpeed mentioned this pull request Jul 20, 2023

[release-4.13] OCPSTRAT-659: DNS: Add awsPrivateHostedZoneRole #1529

Merged

Comments

Conversation

patrickdillon commented May 16, 2023

Uh oh!

openshift-ci bot commented May 16, 2023

Uh oh!

patrickdillon commented May 16, 2023

Uh oh!

patrickdillon commented May 16, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mrunalp commented May 22, 2023

Uh oh!

Uh oh!

Uh oh!

deads2k left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

patrickdillon May 24, 2023

Choose a reason for hiding this comment

Uh oh!

deads2k May 24, 2023

Choose a reason for hiding this comment

Uh oh!

patrickdillon May 26, 2023

Choose a reason for hiding this comment

Uh oh!

patrickdillon commented May 24, 2023

Uh oh!

Uh oh!

Uh oh!

patrickdillon commented May 26, 2023

Uh oh!

deads2k commented May 30, 2023

Uh oh!

Miciah May 30, 2023

Choose a reason for hiding this comment

Uh oh!

patrickdillon May 30, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Miciah May 30, 2023

Choose a reason for hiding this comment

Uh oh!

patrickdillon May 30, 2023

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented May 30, 2023

Uh oh!

Miciah commented May 30, 2023

Uh oh!

openshift-ci bot commented May 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants