-
Notifications
You must be signed in to change notification settings - Fork 459
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
CORS-3440: IngressController subnet selection at installation #1634
base: master
Are you sure you want to change the base?
CORS-3440: IngressController subnet selection at installation #1634
Conversation
@gcs278: This pull request references CORS-3440 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 epic to target the "4.17.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. |
Skipping CI for Draft Pull Request. |
84a2ab7
to
d6a259a
Compare
6a9976a
to
12b9995
Compare
@gcs278: This pull request references CORS-3440 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 epic to target the "4.17.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. |
12b9995
to
8e71d45
Compare
/assign |
/assign @miheer |
This configuration sets the subnets for both the default IngressController and the default | ||
subnets for all new IngressControllers at install time. |
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.
This configuration sets the subnets for both the default IngressController and the default | |
subnets for all new IngressControllers at install time. | |
This proposal allows the install-time configuration of subnets for the default IngressController as well as the default subnets for any new IngressController. |
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.
Done.
subnets for all new IngressControllers at install time. | ||
|
||
This enhancement also deprecates the existing install-config field `platform.aws.subnets` | ||
in favor for a more flexible configuration field that handles both IngressController subnet |
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.
in favor for a more flexible configuration field that handles both IngressController subnet | |
in favor of a more flexible configuration field that handles both IngressController subnet |
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.
Done.
|
||
This enhancement also deprecates the existing install-config field `platform.aws.subnets` | ||
in favor for a more flexible configuration field that handles both IngressController subnet | ||
selection (ingress subnets) and cluster resource subnet selection (cluster subnets). |
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.
We don't have the latter as a use case. Why is it being covered in this proposal?
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.
We discussed in slack, but for completeness: it's an existing feature, the platform.aws.subnets
field in the install-config. I'm giving this type of subnet a proper name for clarity.
Note that I also also changed the "cluster resource subnet" / "cluster subnet" to be "cluster-role subnets", and "ingress subnets" to "IngressController-role subnets" and have included a new ## Definitions and Terminology
section early in the EP to define these, as well as my motivation for why I am call them as such.
a cluster into a VPC with multiple Availability Zones (AZs), but they wish to restrict | ||
their cluster to a single AZ. | ||
|
||
Currently, cluster admins can configure their load balancer subnets by configuring |
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.
Currently, cluster admins can configure their load balancer subnets by configuring | |
Currently, cluster admins can only configure their load balancer subnets after installation, by configuring |
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.
Done. However, with some recent updates, I dropped the "only" because there is another way I've added in a recent update.
However, this approach is only supported as a Day 2 operation, but cluster admins also | ||
need a way to configure ingress subnets at install time (Day 1). |
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.
However, this approach is only supported as a Day 2 operation, but cluster admins also | |
need a way to configure ingress subnets at install time (Day 1). | |
Cluster admins also need a way to configure ingress subnets at install time (Day 1). |
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.
Done.
AZs than the cluster subnets, the default IngressController may map to subnets | ||
in the AZs that are not part of the cluster. This creates a security risk | ||
because the load balancer is mapping to subnets that may belong to other clusters. This |
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.
AZs than the cluster subnets, the default IngressController may map to subnets | |
in the AZs that are not part of the cluster. This creates a security risk | |
because the load balancer is mapping to subnets that may belong to other clusters. This | |
AZs than the cluster subnets, there is no mechanism for the default IngressController | |
to know which subnets and AZs are a part of the cluster. | |
This lack of discoverable information creates a security risk | |
because the load balancer may map to subnets that may belong to other clusters. This |
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.
So this is a bit more nuanced than there is no mechanism for the default IngressController to know which subnets and AZs are a part of the cluster
.
There is a mechanism, it's the kubernetes.io/cluster/<cluster-id>
tag for the subnet. However, it's a combination of the two facts:
- The cluster-admins may not have the ability to change this subnet tag.
- The AWS CCM is very generous in it's selection subnets. If there is no
kubernetes.io/cluster/<cluster-id>
, it selects it. Only when there is an non-matchingkubernetes.io/cluster/<cluster-id>
for another cluster, it will ignore it.
These two reasons may lead to a situation where the IngressController/LoadBalancer is picking up unwanted subnets, and those unwanted subnets could be public subnets when you have a internal/private IngressController.
I realize this subnet selection info is probably worth adding to the EP somewhere, and I'll try to capture your suggestion to be more specific.
|
||
When a cluster admin installs a private cluster into a VPC also containing | ||
public subnets lacking the proper tags, internal LoadBalancer-type services | ||
may inadvertently use both the public and private subnets, unknowingly introducing |
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.
may inadvertently use both the public and private subnets, unknowingly introducing | |
may use the misconfigured public subnets, introducing |
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.
Done.
_"As a user of ROSA HCP, I want to install a cluster with 1 MachinePool in 1 | ||
AZ, then add a 2nd MachinePool in another AZ and update my IngressController's | ||
subnets to map to the new AZ."_ |
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.
This use case is not an installation option, is it? If not, it doesn't belong in this proposal.
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.
It's involves a use case for the IngressConfig, which is introduced in this EP (see last paragraph Additionally, if the user wants all future IngressController to the new subnet by default
), hence why I included it.
|
||
### User Stories | ||
|
||
#### Day 1 Ingress Controller Subnet Selection on AWS |
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.
#### Day 1 Ingress Controller Subnet Selection on AWS | |
#### Day 1 Default Ingress Controller Subnet Selection on AWS |
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.
Done.
- Provide install-time validation for user provided ingress subnets. | ||
- Support configuring the default IngressController's subnets at install time for AWS. | ||
- Support configuring default subnets for all new IngressControllers at install time for AWS. | ||
- Support updating the default subnets for all new IngressControllers. |
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.
Is it possible to be updating at install time?
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.
This goal is for after installation. The goal is for the IngressConfig to be able to be updated after installation so the default subnets for all new IngressController would get updated.
I'll specify "after installation" in this goal.
|
||
- Extend support to platforms other than AWS. | ||
- Automatically restrict IngressController subnet selection for private clusters to ensure the cluster stays private. | ||
- Assume that bring-your-own (BYO) cluster subnets are also ingress subnets by default. |
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.
I feel we shouldn't be mentioning cluster subnets at all in this proposal.
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.
I think it's impossible to not mention them, as we are deprecating & replacing the "cluster subnet" API in this EP, and I think it's important to define the relationship between the two.
2. Add the `Subnets` field to `spec.loadBalancer.platform.aws.subnets` in the IngressConfig | ||
to encode the default subnets for IngressControllers. |
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.
Do you mean we'll end end with a field called spec.loadBalancer.platform.aws.subnets.subnets
? Maybe DefaultSubnets
would be a better choice?
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.
Do you mean we'll end end with a field called spec.loadBalancer.platform.aws.subnets.subnets?
No, good point, updated.
Maybe DefaultSubnets would be a better choice?
It's a good choice, but it would be inconsistent with spec.loadBalancer.platform.aws.subnets.type
, which is also default. Though I like the sound of DefaultSubnets
better, I think breaking consistency with Type
would be confusing to users, as the two fields set the same style of defaults. I actually brought something similar brought up with @miheer's PR: https://github.com/openshift/enhancements/pull/1593/files#r1631413464
The other really unfortunate thing, is that "default" is an overloaded term because of the default
IngressController. Does it mean all default subnets? Or default
IngressController subnets? Or both? In our case, it's both, but my concern is that it's overloaded.
For now, my opinion is that type
, subnets
, and Miheer's new eipAllocations
is acceptable, and consistent.
|
||
### Implementation Details/Notes/Constraints | ||
|
||
As mentioned in [Proposal](#proposal), the install-config and the IngressConfig will be updated |
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.
As mentioned in [Proposal](#proposal), the install-config and the IngressConfig will be updated | |
As mentioned in [the Proposal section](#proposal), the install-config and the IngressConfig will be updated |
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.
Done.
// +kubebuilder:validation:XValidation:rule=`self.all(x, self.exists_one(y, x.id == y.id))`,message="subnetsConfig cannot contain duplicate IDs" | ||
// +kubebuilder:validation:XValidation:rule=`self.exists(x, x.roles.exists(r, r == 'Cluster'))`,message="subnetsConfig must contain at least 1 subnet with the Cluster role" | ||
// +kubebuilder:validation:XValidation:rule=`self.filter(x, x.roles.exists(r, r == 'Ingress')).size() <= 10`,message="subnetsConfig must contain less than 10 subnets with the Ingress role" | ||
// +openshift:enable:FeatureGate=IngressControllerLBSubnetsAWS |
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.
Can you refer to a feature gate in the installer? How can the feature gate be enabled if there is no cluster?
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.
Whoops, no you can't. The installer has the concept of feature sets, but are enforced differently. For example, the NatGatewayOutboundType in Azure is only supported in tech preview. I am just learning how the installer works, so I expect I may get some feedback from them.
Fixed and updated with details on tech preview vs. GA.
// SubnetsConfig specifies the subnet configuration for | ||
// a cluster by specifying a list of subnet ids with their | ||
// designated role. At least one Cluster role subnet must be | ||
// specified. If no Ingress role subnets are specified, the | ||
// IngressController's Load Balancer will automatically discover | ||
// its subnets based on the kubernetes.io/cluster/<cluster-id> tag, | ||
// whether it's public or private, and the availability zone. | ||
// Leave this field unset to have the installer create subnets | ||
// in a new VPC on your behalf. | ||
// | ||
// +optional |
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.
nit - spacing
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.
Done.
// ID specifies the subnet ID of an existing | ||
// subnet. | ||
// | ||
// +required |
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.
spacing
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.
Done.
// Roles specifies the roles (aka functions) that the | ||
// subnet will provide in the cluster. | ||
// |
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.
spacing
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.
Done.
|
||
// SubnetsConfig specifies the subnet configuration for | ||
// a cluster by specifying a list of subnet ids with their | ||
// designated role. At least one Cluster role subnet must be |
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.
I'll come back to review the rest of this after hearing more about the inclusion of Cluster subnets in the proposal.
// Subnets specifies existing subnets (by ID) where cluster | ||
// resources will be created. Leave unset to have the installer | ||
// create subnets in a new VPC on your behalf. | ||
// |
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.
// | |
// |
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.
Done.
8e71d45
to
6a57e4c
Compare
[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 |
Adds enhancements/installer/aws-lb-subnet-selection.md for specifying IngressController's LoadBalancer-type Service subnets at installation time. This enhancement also deprecates the platform.aws.subnets field for a more flexible and extensible API structure.
6a57e4c
to
6a029ed
Compare
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 for the review @candita. I have posted updates: diff here
Mostly addressing your comments, but also:
- Renaming from "cluster subnets" and "ingress subnets" to "cluster-role subnets" and "ingresscontroller-role subnets"
- A
## Definitions and Terminology
section early in the EP - A new alternative
### Assume All Cluster-Role Subnets are IngressController-Role Subnets Alternative
- Referencing the default IngressController by
default
IngressController for extra clarity - TechPreview / GA clarifications
I have not addressed recent updates regarding the IngressController API openshift/api#1841 split from AWSLoadBalancerParameters
to under AWSClassicLoadBalancerParameters
and AWSNetworkLoadBalancerParameters
. I am waiting for some preliminary review from Joel, Miciah, and you on that before I refactor this EP.
openshift/api#1841 and #1595 are the priority at the moment, since they are a dependency of this feature. Thanks.
This configuration sets the subnets for both the default IngressController and the default | ||
subnets for all new IngressControllers at install time. |
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.
Done.
subnets for all new IngressControllers at install time. | ||
|
||
This enhancement also deprecates the existing install-config field `platform.aws.subnets` | ||
in favor for a more flexible configuration field that handles both IngressController subnet |
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.
Done.
|
||
### User Stories | ||
|
||
#### Day 1 Ingress Controller Subnet Selection on AWS |
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.
Done.
|
||
This enhancement also deprecates the existing install-config field `platform.aws.subnets` | ||
in favor for a more flexible configuration field that handles both IngressController subnet | ||
selection (ingress subnets) and cluster resource subnet selection (cluster subnets). |
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.
We discussed in slack, but for completeness: it's an existing feature, the platform.aws.subnets
field in the install-config. I'm giving this type of subnet a proper name for clarity.
Note that I also also changed the "cluster resource subnet" / "cluster subnet" to be "cluster-role subnets", and "ingress subnets" to "IngressController-role subnets" and have included a new ## Definitions and Terminology
section early in the EP to define these, as well as my motivation for why I am call them as such.
a cluster into a VPC with multiple Availability Zones (AZs), but they wish to restrict | ||
their cluster to a single AZ. | ||
|
||
Currently, cluster admins can configure their load balancer subnets by configuring |
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.
Done. However, with some recent updates, I dropped the "only" because there is another way I've added in a recent update.
- Provide install-time validation for user provided ingress subnets. | ||
- Support configuring the default IngressController's subnets at install time for AWS. | ||
- Support configuring default subnets for all new IngressControllers at install time for AWS. | ||
- Support updating the default subnets for all new IngressControllers. |
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.
This goal is for after installation. The goal is for the IngressConfig to be able to be updated after installation so the default subnets for all new IngressController would get updated.
I'll specify "after installation" in this goal.
|
||
- Extend support to platforms other than AWS. | ||
- Automatically restrict IngressController subnet selection for private clusters to ensure the cluster stays private. | ||
- Assume that bring-your-own (BYO) cluster subnets are also ingress subnets by default. |
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.
I think it's impossible to not mention them, as we are deprecating & replacing the "cluster subnet" API in this EP, and I think it's important to define the relationship between the two.
2. Add the `Subnets` field to `spec.loadBalancer.platform.aws.subnets` in the IngressConfig | ||
to encode the default subnets for IngressControllers. |
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.
Do you mean we'll end end with a field called spec.loadBalancer.platform.aws.subnets.subnets?
No, good point, updated.
Maybe DefaultSubnets would be a better choice?
It's a good choice, but it would be inconsistent with spec.loadBalancer.platform.aws.subnets.type
, which is also default. Though I like the sound of DefaultSubnets
better, I think breaking consistency with Type
would be confusing to users, as the two fields set the same style of defaults. I actually brought something similar brought up with @miheer's PR: https://github.com/openshift/enhancements/pull/1593/files#r1631413464
The other really unfortunate thing, is that "default" is an overloaded term because of the default
IngressController. Does it mean all default subnets? Or default
IngressController subnets? Or both? In our case, it's both, but my concern is that it's overloaded.
For now, my opinion is that type
, subnets
, and Miheer's new eipAllocations
is acceptable, and consistent.
_"As a cluster admin, I want to install a cluster with dedicated IngressController | ||
subnets in order to have load balancer VIPs on a dedicated VLAN for applying ACLs."_ |
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.
It's intended for all IngressControllers. Specified.
// +kubebuilder:validation:XValidation:rule=`self.all(x, self.exists_one(y, x.id == y.id))`,message="subnetsConfig cannot contain duplicate IDs" | ||
// +kubebuilder:validation:XValidation:rule=`self.exists(x, x.roles.exists(r, r == 'Cluster'))`,message="subnetsConfig must contain at least 1 subnet with the Cluster role" | ||
// +kubebuilder:validation:XValidation:rule=`self.filter(x, x.roles.exists(r, r == 'Ingress')).size() <= 10`,message="subnetsConfig must contain less than 10 subnets with the Ingress role" | ||
// +openshift:enable:FeatureGate=IngressControllerLBSubnetsAWS |
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.
Whoops, no you can't. The installer has the concept of feature sets, but are enforced differently. For example, the NatGatewayOutboundType in Azure is only supported in tech preview. I am just learning how the installer works, so I expect I may get some feedback from them.
Fixed and updated with details on tech preview vs. GA.
Inactive enhancement proposals go stale after 28d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle stale |
Stale enhancement proposals rot after 7d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle rotten |
Rotten enhancement proposals close after 7d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Reopen the proposal by commenting /close |
@openshift-bot: Closed this PR. 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 kubernetes-sigs/prow repository. |
/reopen |
@gcs278: Reopened this PR. 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 kubernetes-sigs/prow repository. |
@gcs278: This pull request references CORS-3440 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 epic to target the "4.18.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. |
/remove-lifecycle rotten |
@gcs278: all tests passed! 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. |
Adds enhancements/installer/aws-lb-subnet-selection.md for specifying IngressController's LoadBalancer-type Service subnets at installation time.
This enhancement also deprecates the platform.aws.subnets field for a more flexible and extensible API structure.