Use direct cluster ingress #94

sjenning · 2021-03-09T03:41:29Z

Remaining issues:

AWS Instances for the guest cluster are not being added to the ELB created as a result of the router-default Service. I think this is a "Cluster Name" vs "Infra ID" cluster ownership tagging issue. Fixed in Use InfraID as CAPI cluster name #95
We don't deploy the CCO so dns_controller in the ingress operator isn't able to make calls against Route53 Fixed in create OIDC resources and STS assumed roles #111 wire in KAS signing key and issuer URL from IAM create #117 wire in AWS roles #120
Determine how to make e2e pass with this. Not sure how to discover the base-domain in the CI environment.
Implement destroy for private zone

Reviewer notes:

Now cluster name is set at the create infra stage, not create cluster stage. There is a new --base-domain and --name flag on create infra. --base-domain is required while --name will default to example same as create cluster.
create infra will look up the public zone ID based on the base-domain and will lookup/create the private zone based on the name and base-domain

openshift-ci-robot · 2021-03-09T03:41:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sjenning

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [sjenning]

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

sjenning · 2021-03-09T20:00:17Z

This PR is blocked by two things:

Guest cluster instances not added to ELB for Service type LoadBalancer
ingress-operator doesn't have AWS creds for accessing ELB/Router53

refs
https://github.com/openshift/cluster-ingress-operator/blob/630c1cf24a46da73f2dca4f06899b451cf190987/manifests/00-ingress-credentials-request.yaml#L22-L25
https://github.com/openshift/cluster-ingress-operator/blob/630c1cf24a46da73f2dca4f06899b451cf190987/manifests/02-deployment.yaml#L108-L115

derekwaynecarr · 2021-03-10T15:09:02Z

api/v1alpha1/hostedcluster_types.go

@@ -35,6 +35,20 @@ type HostedClusterSpec struct {

 	// InfraID is used to identify the cluster in cloud platforms
 	InfraID string `json:"infraID,omitempty"`
+
+	// DNS configuration for the cluster


nit: isnt this for the ingress controller, not the cluster itself?

@derekwaynecarr It is for the ingress controller. Which makes me think, should we allow you to specify an arbitrary list of ingress controllers like Hive does?

If this is for ingress, and if there are multiple ingresses configured (as @csrwng asks), does that mean each ingress's DNS zone is defined independently?

What fields are mutable? What is the behavior if they are mutated?

Yes, this just configures the default ingress controller

can we reflect this expectation either in a comment or the API field name?

cmd/infra/aws/create.go

enxebre · 2021-03-30T14:28:56Z

cmd/cluster/create.go

@@ -174,7 +180,7 @@ func CreateCluster(ctx context.Context, opts Options) error {

 	exampleObjects := apifixtures.ExampleOptions{
 		Namespace:        opts.Namespace,
-		Name:             opts.Name,
+		Name:             infra.Name,


just to clarify, Is it a hard requirement for the cluster.Name to be the same than the infra.Name? or is it only for the BaseDomain and the infraID?

The cluster Name is independent the InfraID. The Name and BaseDomain are used to determine the DNS private zone.

It's not clear to me still why ExampleOptions.Name is now being changed to infra.Name. Isn't ExampleOptions.Name supposed to equal the HostedCluster name (represented here by opts.Name)?

enxebre · 2021-03-30T14:30:58Z

cmd/infra/aws/create.go

 	}

 	cmd.Flags().StringVar(&opts.InfraID, "infra-id", opts.InfraID, "Cluster ID with which to tag AWS resources (required)")
 	cmd.Flags().StringVar(&opts.AWSCredentialsFile, "aws-creds", opts.AWSCredentialsFile, "Path to an AWS credentials file (required)")
 	cmd.Flags().StringVar(&opts.OutputFile, "output-file", opts.OutputFile, "Path to file that will contain output information from infra resources (optional)")
 	cmd.Flags().StringVar(&opts.Region, "region", opts.Region, "Region where cluster infra should be created")
 	cmd.Flags().StringSliceVar(&opts.AdditionalTags, "additional-tags", opts.AdditionalTags, "Additional tags to set on AWS resources")
+	cmd.Flags().StringVar(&opts.Name, "name", opts.Name, "A name for the cluster")


may be clarify what a name for the cluster means? e.g "this is added as a subdomain when creating ingress resources"

This is the same flag you would pass to create cluster and I copied the description from there hoping people would understand they are the same thing. While we only use the name for DNS at create infra time, we use it for other things at create cluster time e.g. determining the namespace for the HCP.

enxebre · 2021-03-30T14:34:58Z

cmd/infra/aws/create.go

+	awsConfig := &aws.Config{
+		// Route53 is weird about regions
+		// https://github.com/openshift/cluster-ingress-operator/blob/5660b43d66bd63bbe2dcb45fb40df98d8d91347e/pkg/dns/aws/dns.go#L163-L169
+		Region: aws.String("us-east-1"),


indeed 😦

enxebre · 2021-03-30T14:40:27Z

cmd/infra/aws/route53.go

+
+func (o *CreateInfraOptions) CreatePrivateZone(client route53iface.Route53API, vpcID string) (string, error) {
+	name := fmt.Sprintf("%s.%s", o.Name, o.BaseDomain)
+	id, err := lookupZone(client, name, true)


nit: can inline the if sentence here since variables are only used within its context

enxebre · 2021-03-30T15:37:37Z

cmd/cluster/create.go

@@ -85,6 +88,7 @@ func NewCreateCommand() *cobra.Command {
 	cmd.Flags().StringVar(&opts.Region, "region", opts.Region, "Region to use for AWS infrastructure.")
 	cmd.Flags().StringVar(&opts.InfraID, "infra-id", opts.InfraID, "Infrastructure ID to use for AWS resources.")


orthogonal to this PR, we should clarify this must be unique for the each cluster living in the same aws account

ironcladlou · 2021-03-30T20:00:02Z

api/v1alpha1/hostedcluster_types.go

+
+// DNSSpec specifies the DNS configuration in the cluster
+type DNSSpec struct {
+	// BaseDomain is the base domain of the cluster.


We need to figure out how to specify this unambiguously.

What exactly does it represent? e.g.

The canonical domain suffix for any generated records for this cluster?

The suffix of a computed canonical name (e.g. name+basedomain)?

Is it a canonical FQDN? If not, how is the canonical value computed?

The current implementation seems to imply this is not a canonical FQDN

Must BaseDomain be a subdomain of the domain represented by PublicZoneId/PrivateZoneID?

What are the uniqueness constraints?

If there are uniqueness constraints, it's implied the value on spec is not usable absent some sort of async validation process

Where is that validation result surfaced on status?

What field will our controllers read given the spec value isn't necessarily valid?

Does this imply some sort of valid or admitted condition for HostedClusters generally the API should describe and the operator must honor?

If ingress is considered required, I'm not entirely sure how to separate the notion this is configuration for "the ingress controller" and not for the cluster itself (https://github.com/openshift/hypershift/pull/94/files#r591603025). Seems like a distinction without a difference?

ironcladlou · 2021-03-30T20:28:42Z

api/v1alpha1/hostedcluster_types.go

+	// PublicZoneID is the Hosted Zone ID where all the DNS records that are publicly accessible to
+	// the internet exist.
+	// +optional
+	PublicZoneID string `json:"publicZoneID,omitempty"`


Along the lines of the above, what doe these represent? e.g. Are they assumed to be authoritative for BaseDomain?

sjenning · 2021-03-31T16:58:58Z

/test e2e-aws

sjenning · 2021-03-31T18:09:48Z

/test e2e-aws

sjenning · 2021-03-31T22:03:21Z

/test e2e-aws

csrwng · 2021-03-31T23:05:54Z

tagging and will address pending issues as a follow up
/lgtm

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 9, 2021

openshift-ci-robot requested review from csrwng and enxebre March 9, 2021 03:41

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 9, 2021

sjenning force-pushed the direct-cluster-ingress branch 3 times, most recently from d9a61f0 to efe7925 Compare March 9, 2021 15:39

derekwaynecarr reviewed Mar 10, 2021

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 13, 2021

This was referenced Mar 17, 2021

wire in KAS signing key and issuer URL from IAM create #117

Merged

wire in AWS roles #120

Merged

sjenning force-pushed the direct-cluster-ingress branch from efe7925 to ebc39f7 Compare March 18, 2021 19:53

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 18, 2021

sjenning force-pushed the direct-cluster-ingress branch from ebc39f7 to 2981e3d Compare March 19, 2021 00:26

ironcladlou reviewed Mar 19, 2021

View reviewed changes

cmd/infra/aws/create.go Outdated Show resolved Hide resolved

sjenning force-pushed the direct-cluster-ingress branch from 2981e3d to feeed78 Compare March 19, 2021 14:35

sjenning force-pushed the direct-cluster-ingress branch from feeed78 to 0f68f9e Compare March 30, 2021 03:43

enxebre reviewed Mar 30, 2021

View reviewed changes

ironcladlou reviewed Mar 30, 2021

View reviewed changes

sjenning force-pushed the direct-cluster-ingress branch 2 times, most recently from f47a3d5 to 84df1b8 Compare March 31, 2021 16:58

sjenning changed the title ~~WIP: use direct cluster ingress~~ Use direct cluster ingress Mar 31, 2021

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 31, 2021

sjenning force-pushed the direct-cluster-ingress branch from 84df1b8 to 2e9fd73 Compare March 31, 2021 18:09

use direct cluster ingress

3add1d0

sjenning force-pushed the direct-cluster-ingress branch from 2e9fd73 to 3add1d0 Compare March 31, 2021 22:03

openshift-ci-robot assigned csrwng Mar 31, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 31, 2021

openshift-merge-robot merged commit c166356 into openshift:main Mar 31, 2021

sjenning deleted the direct-cluster-ingress branch April 1, 2021 16:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use direct cluster ingress #94

Use direct cluster ingress #94

sjenning commented Mar 9, 2021 •

edited

openshift-ci-robot commented Mar 9, 2021

sjenning commented Mar 9, 2021 •

edited

derekwaynecarr Mar 10, 2021

csrwng Mar 10, 2021

ironcladlou Mar 11, 2021

sjenning Mar 19, 2021

enxebre Mar 30, 2021

enxebre Mar 30, 2021 •

edited

sjenning Mar 30, 2021

ironcladlou Mar 30, 2021

enxebre Mar 30, 2021 •

edited

sjenning Mar 30, 2021

enxebre Mar 30, 2021

sjenning Mar 30, 2021

enxebre Mar 30, 2021

enxebre Mar 30, 2021

ironcladlou Mar 30, 2021 •

edited

ironcladlou Mar 30, 2021

ironcladlou Mar 30, 2021

sjenning commented Mar 31, 2021

sjenning commented Mar 31, 2021

sjenning commented Mar 31, 2021

csrwng commented Mar 31, 2021

		@@ -85,6 +88,7 @@ func NewCreateCommand() *cobra.Command {
		cmd.Flags().StringVar(&opts.Region, "region", opts.Region, "Region to use for AWS infrastructure.")
		cmd.Flags().StringVar(&opts.InfraID, "infra-id", opts.InfraID, "Infrastructure ID to use for AWS resources.")

Use direct cluster ingress #94

Use direct cluster ingress #94

Conversation

sjenning commented Mar 9, 2021 • edited

openshift-ci-robot commented Mar 9, 2021

sjenning commented Mar 9, 2021 • edited

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

enxebre Mar 30, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre Mar 30, 2021 • edited

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

Choose a reason for hiding this comment

ironcladlou Mar 30, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjenning commented Mar 31, 2021

sjenning commented Mar 31, 2021

sjenning commented Mar 31, 2021

csrwng commented Mar 31, 2021

sjenning commented Mar 9, 2021 •

edited

sjenning commented Mar 9, 2021 •

edited

enxebre Mar 30, 2021 •

edited

enxebre Mar 30, 2021 •

edited

ironcladlou Mar 30, 2021 •

edited