Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Add EKS Control Plane private subnet restriction flag #5058

Merged
merged 1 commit into from
Jul 29, 2024

Conversation

jas-nik
Copy link
Contributor

@jas-nik jas-nik commented Jul 15, 2024

What type of PR is this?
/kind feature

Add flag to enable private subnet filter for EKS Control Plane network configuration

What this PR does / why we need it:

In a scenario where Bastion host needs to be provisioned, a public subnet is required to be specified in Control Plane spec. Specifying the public subnet in the Control Plane Network spec will attach it to the EKS Control Plane. For security reasons, users might not prefer to attach public subnets to EKS control plane. This flag will provide the ability to restrict only the private subnets attachment to EKS control plane even if public subnets are specified in the EKS Control Plane Network configuration.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Checklist:

  • squashed commits
  • includes documentation
  • includes emojis
  • adds unit tests
  • adds or updates e2e tests

Release note:

- Flag to enable private subnet filter for EKS control plane

cc: @richardcase

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Jul 15, 2024
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 15, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @jas-nik. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@jas-nik
Copy link
Contributor Author

jas-nik commented Jul 15, 2024

/cc @richardcase

@k8s-ci-robot k8s-ci-robot requested a review from richardcase July 15, 2024 22:18
@richardcase
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 17, 2024
func (r *AWSManagedControlPlane) validateFilterPrivateSubnets() field.ErrorList {
var allErrs field.ErrorList

if r.Spec.FilterPrivateSubnets {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should do this check here. The subnet details are populated during reconciliation (if you don't explicitly set them) and so there will be no private subnets. We could do this check as part of the reconciliation instead,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you link me to the reconcile logic? Are you referring to the cluster.go reconcileVpcConfig?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pkg/cloud/scope/managedcontrolplane.go Outdated Show resolved Hide resolved
@jas-nik jas-nik changed the title ✨ Add EKS Control Plane Private Subnet Filter ✨ Add EKS Control Plane Private Subnet restriction flag Jul 22, 2024
@jas-nik jas-nik changed the title ✨ Add EKS Control Plane Private Subnet restriction flag ✨ Add EKS Control Plane private subnet restriction flag Jul 22, 2024
@richardcase
Copy link
Member

/milestone v2.6.0

@k8s-ci-robot k8s-ci-robot added this to the v2.6.0 milestone Jul 22, 2024
Copy link
Contributor

@alexander-demicev alexander-demicev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 23, 2024
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 23, 2024
@jas-nik
Copy link
Contributor Author

jas-nik commented Jul 23, 2024

@richardcase TestFuzzyConversion seems to be failing without updating v1beta1 awsmanagedcontrolplane spec with the RestrictPrivateSubnets flag

   conversion.go:241:
          &v1beta2.AWSManagedControlPlane{
          	TypeMeta:   {},
          	ObjectMeta: {Name: "WČ皚蓊żC躟Xƻz甹義ɐ_", GenerateName: `\f7芃ɍm尰±ğ愅q鱁炭Ô荌Ŗ洀`, Namespace: "媮wJŜø,ì¸郌ʮYHȈ!", SelfLink: "Ƙ挊苹沚{BɜO6WN嘖嫿跽Rm", ...},
          	Spec: v1beta2.AWSManagedControlPlaneSpec{
          		... // 23 identical fields
          		OIDCIdentityProviderConfig: &{ClientID: "掠糼8+3ȱ镯鱁B+Ǜ", GroupsClaim: &"先Ʌ禓鑠ĢȮǍ鑈僨y闉", IdentityProviderConfigName: "蠝?硻Ğƴs]嶨/礓G<鳮歜žFkêA", IssuerURL: "ÿö顕Ųɻ¶誸ȿc羄窜蜫<ǦǣŝȤ", ...},
          		VpcCni:                     {Env: {}},
        - 		RestrictPrivateSubnets:     true,
        + 		RestrictPrivateSubnets:     false,
          		KubeProxy:                  {},
          	},
          	Status: {Network: {APIServerELB: {ARN: `"÷岥_Cɩ`, Name: "铱5髄", DNSName: "ɴ贬ǔc%!M(MISSING) 墏薩FȪ钮后ɚ算âs", Scheme: s"DŽY", ...}, SecondaryAPIServerELB: {ARN: "ĈȸR%!胩(MISSING)@ńnZH9腣M顷", Name: "c", DNSName: "", Scheme: s"ɞ釛FŴ嚾dz|筻ŰD^", ...}}, Bastion: &{ID: "薞梮怐ń6ĠņƂ8[ɧƪ;楡", State: "釴lƪX-l½@ɞȊȴTf罏c", Type: "p甞ONj鷂ƣ蔟iH疖漫åO-欮d焾祄", SubnetID: `褦龁ȌƆŤy颤ġɑ,\QƸǞtčʡȇ`, ...}, OIDCProvider: {ARN: "7稔湩@n筲沤0#ƶ潴鸟ƷQ", TrustPolicy: `a垯dzqƍƼÁ蹏ʦ鯚ĩ\ŭ猔]Õ侵Ɍ`}, Addons: {{Name: "俥ʘ&nIã4", Version: "靽,HO鰒廥;铢鮌磴娟釫*", ARN: "D5醦躴_ā秚胕Ȳ", ServiceAccountRoleArn: &"ǎ", ...}}, ...},
          }

        Expected
            <bool>: false
        to be true
--- FAIL: TestFuzzyConversion (3.92s)
    --- FAIL: TestFuzzyConversion/for_AWSManagedControlPlane (3.91s)
        --- PASS: TestFuzzyConversion/for_AWSManagedControlPlane/spoke-hub-spoke (3.91s)
        --- FAIL: TestFuzzyConversion/for_AWSManagedControlPlane/hub-spoke-hub (0.00s)
FAIL
FAIL	sigs.k8s.io/cluster-api-provider-aws/v2/controlplane/eks/api/v1beta1	4.073s

Copy link
Contributor

@cnmcavoy cnmcavoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/hold

controlplane/eks/api/v1beta1/zz_generated.conversion.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 23, 2024
@cnmcavoy
Copy link
Contributor

/remove-hold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 24, 2024
@cnmcavoy
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 24, 2024
@richardcase
Copy link
Member

@richardcase TestFuzzyConversion seems to be failing without updating v1beta1 awsmanagedcontrolplane spec with the RestrictPrivateSubnets flag

I see you got it sorted. The Fuzzy tests are a pain when you first enouncter them.

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: richardcase

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 29, 2024
@k8s-ci-robot k8s-ci-robot merged commit 8d68d03 into kubernetes-sigs:main Jul 29, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants