✨ controlplane/rosa: allow configuring private link #4758

stevekuznetsov · 2024-01-24T17:51:40Z

controlplane/rosa: add a test for the cluster transform

Signed-off-by: Steve Kuznetsov skuznets@redhat.com

controlpane/rosa: make cluster declaration idomatic

Signed-off-by: Steve Kuznetsov skuznets@redhat.com

controlplane/rosa: allow configuring private link

Signed-off-by: Steve Kuznetsov skuznets@redhat.com

NONE

stevekuznetsov · 2024-01-24T17:54:03Z

/assign @vincepri @muraee

k8s-ci-robot · 2024-01-24T17:54:06Z

@stevekuznetsov: GitHub didn't allow me to assign the following users: muraee.

Note that only kubernetes-sigs members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @vincepri @muraee

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.

stevekuznetsov · 2024-01-24T17:55:11Z

controlplane/rosa/api/v1beta2/rosacontrolplane_types.go

@@ -74,6 +74,21 @@ type RosaControlPlaneSpec struct { //nolint: maligned
 	// - ocmApiUrl: Optional, defaults to 'https://api.openshift.com'
 	// +optional
 	CredentialsSecretRef *corev1.LocalObjectReference `json:"credentialsSecretRef,omitempty"`
+
+	AWS *AWSConfiguration `json:"aws,omitempty"`


Our Spec doesn't really mirror the fields in the ROSA API - I don't see a good reason to diverge from theirs, having to remember the mapping from one to the other is extra mental overhead and as we are, we're putting a bunch of concerns into the same namespace as opposed to having higher-level fields to separate them out.

To that end, I made this mirror the API we're talking with and will move bits over one by one until we're more similar.

k8s-ci-robot · 2024-01-24T18:44:49Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from vincepri. 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

stevekuznetsov · 2024-01-24T19:41:55Z

Not able to reproduce either of those failures locally ...

$ make manager-aws-infrastructure 
CGO_ENABLED=0 GOARCH=amd64 go build -ldflags "-X 'sigs.k8s.io/cluster-api-provider-aws/v2/version.buildDate=2024-01-24T19:41:14Z' -X 'sigs.k8s.io/cluster-api-provider-aws/v2/version.gitCommit=613908e28545309cc46dad791ee6f78c0f07d79c' -X 'sigs.k8s.io/cluster-api-provider-aws/v2/version.gitTreeState=clean' -X 'sigs.k8s.io/cluster-api-provider-aws/v2/version.gitMajor=2' -X 'sigs.k8s.io/cluster-api-provider-aws/v2/version.gitMinor=3' -X 'sigs.k8s.io/cluster-api-provider-aws/v2/version.gitVersion=v2.3.0-82-613908e2854530' -X 'sigs.k8s.io/cluster-api-provider-aws/v2/version.gitReleaseCommit=2562a8bc41a9b76a25dd6b4f6ba9252a033f5ba1' -extldflags '-static'" -o bin/manager .
$ echo $?
0
$ make lint
hack/tools/bin/golangci-lint run -v --fast=false 
INFO [config_reader] Config search paths: [./ /home/stevekuznetsov/code/kubernetes-sigs/cluster-api-provider-aws/src/sigs.k8s.io/cluster-api-provider-aws /home/stevekuznetsov/code/kubernetes-sigs/cluster-api-provider-aws/src/sigs.k8s.io /home/stevekuznetsov/code/kubernetes-sigs/cluster-api-provider-aws/src /home/stevekuznetsov/code/kubernetes-sigs/cluster-api-provider-aws /home/stevekuznetsov/code/kubernetes-sigs /home/stevekuznetsov/code /home/stevekuznetsov /home /] 
INFO [config_reader] Used config file .golangci.yml 
INFO [lintersdb] Active 61 linters: [asasalint asciicheck bodyclose containedctx decorder depguard dogsled dupword errcheck errchkjson execinquery exportloopref gci ginkgolinter gocheckcompilerdirectives gochecksumtype goconst gocritic gocyclo godot gofmt goheader goimports goprintffuncname gosec gosimple gosmopolitan govet grouper importas inamedparam ineffassign loggercheck maintidx mirror misspell nakedret nilerr noctx nolintlint nosprintfhostport perfsprint prealloc predeclared protogetter reassign revive rowserrcheck sloglint staticcheck stylecheck tagalign testableexamples testifylint thelper unconvert unparam unused usestdlibvars whitespace zerologlint] 
INFO [loader] Using build tags: [tools e2e]       
INFO [loader] Go packages loading at mode 575 (compiled_files|imports|name|types_sizes|deps|exports_file|files) took 2.366565759s 
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 59.583945ms 
INFO [linters_context/goanalysis] analyzers took 2m3.43480797s with top 10 stages: buildir: 19.15989136s, buildssa: 9.727010505s, the_only_name: 3.690965596s, gci: 3.338577862s, unconvert: 3.111333173s, unparam: 2.582898125s, goimports: 2.457682343s, S1038: 1.748988796s, SA4030: 1.52631543s, S1039: 1.473506917s 
INFO [runner] Issues before processing: 3464, after processing: 0 
INFO [runner] Processors filtering stat (out/in): autogenerated_exclude: 1008/1017, exclude-rules: 170/378, filename_unadjuster: 3464/3464, identifier_marker: 1008/1008, skip_dirs: 1017/1017, cgo: 3464/3464, skip_files: 1017/3464, exclude: 378/1008, nolint: 0/170, path_prettifier: 3464/3464 
INFO [runner] processing took 50.778344ms with stages: nolint: 20.978283ms, identifier_marker: 8.928472ms, path_prettifier: 6.714796ms, autogenerated_exclude: 4.216595ms, exclude-rules: 4.194967ms, exclude: 2.781351ms, skip_files: 1.918012ms, skip_dirs: 785.863µs, cgo: 165.097µs, filename_unadjuster: 90.228µs, max_same_issues: 1.769µs, uniq_by_line: 920ns, path_shortener: 658ns, fixer: 285ns, diff: 246ns, source_code: 197ns, severity-rules: 191ns, sort_results: 159ns, max_from_linter: 116ns, path_prefixer: 70ns, max_per_file_from_linter: 69ns 
INFO [runner] linters took 3.845419901s with stages: goanalysis_metalinter: 3.794551852s 
INFO File cache stats: 257 entries of total size 2.5MiB 
INFO Memory: 62 samples, avg is 1722.6MB, max is 4926.1MB 
INFO Execution took 6.27549161s                   
$ echo $?
0

stevekuznetsov · 2024-01-24T19:42:03Z

/retest

stevekuznetsov · 2024-01-24T19:42:55Z

Makefile

@@ -76,7 +76,7 @@ DOCKER_BUILDKIT=1
 export ACK_GINKGO_DEPRECATIONS := 1.16.4

 # Set --output-base for conversion-gen if we are not within GOPATH
-ifneq ($(abspath $(REPO_ROOT)),$(shell go env GOPATH)/src/sigs.k8s.io/cluster-api-provider-aws)
+ifneq ($(abspath $(REPO_ROOT)),$(abspath $(shell go env GOPATH)/src/sigs.k8s.io/cluster-api-provider-aws))


Previously Makefile couldn't handle $GOPATH with trailing / ...

We should not be creating aliases for APIs that are something *other* than the name of the package and its version. Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

nrb · 2024-01-24T22:35:39Z

/test pull-cluster-api-provider-aws-e2e

nrb · 2024-01-24T22:38:01Z

/test pull-cluster-api-provider-aws-e2e-conformance

muraee · 2024-01-25T16:49:47Z

controlplane/rosa/controllers/rosacontrolplane_controller.go

+				AccountID(*controlPlane.Spec.AccountID).
+				BillingAccountID(*controlPlane.Spec.AccountID).
+				SubnetIDs(controlPlane.Spec.Subnets...).
+				PrivateLink(controlPlane.Spec.AWS.PrivateLink).


if PrivateLink is enabled we should set the API server listening mode to Internal
see https://github.com/openshift/rosa/blob/4639f9824693ae52d013716a2b6b47758180eaea/pkg/ocm/clusters.go#L1041-L1049

muraee · 2024-01-25T16:52:41Z

controlplane/rosa/api/v1beta2/rosacontrolplane_types.go

@@ -74,6 +74,22 @@ type RosaControlPlaneSpec struct { //nolint: maligned
 	// - ocmApiUrl: Optional, defaults to 'https://api.openshift.com'
 	// +optional
 	CredentialsSecretRef *corev1.LocalObjectReference `json:"credentialsSecretRef,omitempty"`
+
+	// AWS configures aspects of the ROSA HCP workload cluster that are specific to AWS.
+	AWS AWSConfiguration `json:"aws"`


why is this field required?
Given this provider can only provision AWS clusters, the naming of this field aws might be confusing. Do we really need to wrap the PrivateLink field with this?

#4758 (comment)

muraee · 2024-01-25T16:53:21Z

controlplane/rosa/api/v1beta2/rosacontrolplane_types.go

+	PrivateLink bool `json:"privateLink"`
+
+	// PrivateLinkConfiguration configures the Private Link for the cluster
+	PrivateLinkConfiguration *PrivateLinkConfiguration `json:"privateLinkConfiguration,omitempty"`


PrivateLinkConfiguration is not exposed by the rosa cli, I don't think we should expose it here.

How does someone configure the principals with the rosa CLI? Is it day-2?

I don't think its possible to configure this, maybe only SREs have access.

k8s-ci-robot · 2024-01-25T19:25:53Z

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.

stevekuznetsov · 2024-02-29T02:13:43Z

Superseded by #4832

/close

k8s-ci-robot · 2024-02-29T02:13:47Z

@stevekuznetsov: Closed this PR.

In response to this:

Superseded by #4832

/close

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.

k8s-ci-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Jan 24, 2024

k8s-ci-robot requested review from AndiDog and cnmcavoy January 24, 2024 17:51

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 24, 2024

stevekuznetsov mentioned this pull request Jan 24, 2024

ROSA: Extend ROSAControlPlane API #4737

Closed

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jan 24, 2024

k8s-ci-robot assigned vincepri Jan 24, 2024

stevekuznetsov commented Jan 24, 2024

View reviewed changes

stevekuznetsov force-pushed the skuznets/rosa-control-plane-private-link branch from 0821b5b to 69ec498 Compare January 24, 2024 18:44

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 24, 2024

stevekuznetsov force-pushed the skuznets/rosa-control-plane-private-link branch from 69ec498 to 613908e Compare January 24, 2024 19:25

stevekuznetsov commented Jan 24, 2024

View reviewed changes

stevekuznetsov force-pushed the skuznets/rosa-control-plane-private-link branch from 613908e to 0b3474e Compare January 24, 2024 19:43

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 24, 2024

stevekuznetsov force-pushed the skuznets/rosa-control-plane-private-link branch 2 times, most recently from 4f94515 to e4d46ba Compare January 24, 2024 20:16

stevekuznetsov added 4 commits January 24, 2024 13:52

golangci: importas: use correct import for the OCM API

7bb8ea0

We should not be creating aliases for APIs that are something *other* than the name of the package and its version. Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

controlplane/rosa: isolate CAPI -> OCM object transform

2c62cf9

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

controlplane/rosa: add a test for the CAPI -> OCM transform

46eda01

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

controlpane/rosa: make cluster declaration idomatic

e278d2e

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

controlplane/rosa: support Private Link

3fbec96

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

stevekuznetsov force-pushed the skuznets/rosa-control-plane-private-link branch from e4d46ba to 3fbec96 Compare January 24, 2024 21:12

muraee reviewed Jan 25, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 25, 2024

stevekuznetsov mentioned this pull request Feb 29, 2024

✨ rosa: add missing fields to spec #4832

Closed

k8s-ci-robot closed this Feb 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ controlplane/rosa: allow configuring private link #4758

✨ controlplane/rosa: allow configuring private link #4758

stevekuznetsov commented Jan 24, 2024 •

edited

Loading

stevekuznetsov commented Jan 24, 2024

k8s-ci-robot commented Jan 24, 2024

stevekuznetsov Jan 24, 2024 •

edited

Loading

k8s-ci-robot commented Jan 24, 2024

stevekuznetsov commented Jan 24, 2024

stevekuznetsov commented Jan 24, 2024

stevekuznetsov Jan 24, 2024

nrb commented Jan 24, 2024

nrb commented Jan 24, 2024

muraee Jan 25, 2024

muraee Jan 25, 2024

stevekuznetsov Jan 25, 2024

muraee Jan 25, 2024

stevekuznetsov Jan 25, 2024

muraee Jan 25, 2024

k8s-ci-robot commented Jan 25, 2024

stevekuznetsov commented Feb 29, 2024

k8s-ci-robot commented Feb 29, 2024

✨ controlplane/rosa: allow configuring private link #4758

✨ controlplane/rosa: allow configuring private link #4758

Conversation

stevekuznetsov commented Jan 24, 2024 • edited Loading

stevekuznetsov commented Jan 24, 2024

k8s-ci-robot commented Jan 24, 2024

stevekuznetsov Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 24, 2024

stevekuznetsov commented Jan 24, 2024

stevekuznetsov commented Jan 24, 2024

stevekuznetsov Jan 24, 2024

Choose a reason for hiding this comment

nrb commented Jan 24, 2024

nrb commented Jan 24, 2024

muraee Jan 25, 2024

Choose a reason for hiding this comment

muraee Jan 25, 2024

Choose a reason for hiding this comment

stevekuznetsov Jan 25, 2024

Choose a reason for hiding this comment

muraee Jan 25, 2024

Choose a reason for hiding this comment

stevekuznetsov Jan 25, 2024

Choose a reason for hiding this comment

muraee Jan 25, 2024

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 25, 2024

stevekuznetsov commented Feb 29, 2024

k8s-ci-robot commented Feb 29, 2024

stevekuznetsov commented Jan 24, 2024 •

edited

Loading

stevekuznetsov Jan 24, 2024 •

edited

Loading