-
Notifications
You must be signed in to change notification settings - Fork 675
✨ Add v1beta2 conditions infrastructure to AWSCluster #5854
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
0a79592
2dc7df7
4eb8d9e
48182c1
ad57c6a
44772a3
b97102d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -281,6 +281,23 @@ type AWSClusterStatus struct { | |
| FailureDomains clusterv1beta1.FailureDomains `json:"failureDomains,omitempty"` | ||
| Bastion *Instance `json:"bastion,omitempty"` | ||
| Conditions clusterv1beta1.Conditions `json:"conditions,omitempty"` | ||
|
|
||
| // v1beta3 groups all the fields that will be added or modified in AWSCluster's status with the V1Beta3 version. | ||
| // +optional | ||
| V1Beta3 *AWSClusterV1Beta3Status `json:"v1beta3,omitempty"` | ||
| } | ||
|
|
||
| // AWSClusterV1Beta3Status groups all the fields that will be added or modified in AWSClusterStatus with the V1Beta3 version. | ||
| // See https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/proposals/20240916-improve-status-in-CAPI-resources.md for more context. | ||
| type AWSClusterV1Beta3Status struct { | ||
| // Conditions represents the observations of an AWSCluster's current state. | ||
| // Known condition types are Ready, VpcReady, SubnetsReady, InternetGatewayReady, NatGatewaysReady, | ||
| // RouteTablesReady, ClusterSecurityGroupsReady, BastionHostReady, LoadBalancerReady, and Paused. | ||
| // +optional | ||
| // +listType=map | ||
| // +listMapKey=type | ||
| // +kubebuilder:validation:MaxItems=32 | ||
| Conditions []metav1.Condition `json:"conditions,omitempty"` | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we only dealing with AWSCluster and Conditions here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. my bad, i was initially planning to split these into separate prs to keep things smaller,. i agree it’s better to avoid creating too many prs for closely related changes.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rethinking again, once we settle on the approach for v1beta1 conversions, #5854 (comment) i’ll add the same
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm happy either way. Smaller PRs always helps with review though. |
||
| } | ||
|
|
||
| // S3Bucket defines a supporting S3 bucket for the cluster, currently can be optionally used for Ignition. | ||
|
|
@@ -355,6 +372,22 @@ func (r *AWSCluster) SetConditions(conditions clusterv1beta1.Conditions) { | |
| r.Status.Conditions = conditions | ||
| } | ||
|
|
||
| // GetV1Beta3Conditions returns the set of conditions for this object. | ||
| func (r *AWSCluster) GetV1Beta3Conditions() []metav1.Condition { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is where the naming of things is going to get weird 😉 We need to adopt // SetV1Beta2Conditions sets conditions for an API object.
// Note: SetV1Beta2Conditions will be renamed to SetConditions in a later stage of the transition to V1Beta2.
SetV1Beta2Conditions([]metav1.Condition)
// GetV1Beta2Conditions returns the list of conditions for a cluster API object.
// Note: GetV1Beta2Conditions will be renamed to GetConditions in a later stage of the transition to V1Beta2.
GetV1Beta2Conditions() []metav1.ConditionOr we need additional functions. It will be very messy/weird until we drop the old fields.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks for clarifying 😄 it does feel a bit awkward during the transition, but i agree it’s better to align exactly with capi’s interface expectations. |
||
| if r.Status.V1Beta3 == nil { | ||
| return nil | ||
| } | ||
| return r.Status.V1Beta3.Conditions | ||
| } | ||
|
|
||
| // SetV1Beta3Conditions sets conditions for an API object. | ||
| func (r *AWSCluster) SetV1Beta3Conditions(conditions []metav1.Condition) { | ||
| if r.Status.V1Beta3 == nil { | ||
| r.Status.V1Beta3 = &AWSClusterV1Beta3Status{} | ||
| } | ||
| r.Status.V1Beta3.Conditions = conditions | ||
| } | ||
|
|
||
| func init() { | ||
| SchemeBuilder.Register(&AWSCluster{}, &AWSClusterList{}) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,114 @@ | ||
| /* | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are these constants introduced? They are not being used anywhere and most values are duplicating existing constants.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm, make sense.. i initially added these constants as groundwork for the next step, where the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will introduce them together with the controller changes in the follow-up pr where they’ll actually be used.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great thanks! Makes sense to introduce them in the next step. Also only introducing what is necessary.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at the initial comment from @damdo, he is saying you should not update files under The comment is not asking to change CAPI conversion, just to not include it in the older CAPA v1beta1 API. Please bear in mind that CAPI v1beta1 and CAPA v1beta1 are 2 different things.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks @clebs for the review will keep this in mind. |
||
| Copyright 2022 The Kubernetes Authors. | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| package v1beta2 | ||
|
|
||
| import clusterv1beta1 "sigs.k8s.io/cluster-api/api/core/v1beta1" | ||
|
|
||
| // AWSCluster's v1beta3 conditions and corresponding reasons. | ||
| // These will be used with the V1Beta3 API version. | ||
|
|
||
| // AWSCluster's Ready condition and corresponding reasons that will be used in v1Beta3 API version. | ||
| const ( | ||
| // AWSClusterReadyCondition is true if the AWSCluster's deletionTimestamp is not set, and all | ||
| // infrastructure conditions (VpcReady, SubnetsReady, etc.) are true. | ||
| AWSClusterReadyCondition = clusterv1beta1.ReadyV1Beta2Condition | ||
|
|
||
| // AWSClusterReadyReason surfaces when the AWSCluster readiness criteria is met. | ||
| AWSClusterReadyReason = clusterv1beta1.ReadyV1Beta2Reason | ||
|
|
||
| // AWSClusterNotReadyReason surfaces when the AWSCluster readiness criteria is not met. | ||
| AWSClusterNotReadyReason = clusterv1beta1.NotReadyV1Beta2Reason | ||
|
|
||
| // AWSClusterReadyUnknownReason surfaces when at least one AWSCluster readiness criteria is unknown | ||
| // and no AWSCluster readiness criteria is not met. | ||
| AWSClusterReadyUnknownReason = clusterv1beta1.ReadyUnknownV1Beta2Reason | ||
| ) | ||
|
|
||
| // AWSCluster's VpcReady condition and corresponding reasons that will be used in v1Beta3 API version. | ||
| const ( | ||
| // AWSClusterVpcReadyCondition documents the status of the VPC for an AWSCluster. | ||
| AWSClusterVpcReadyCondition = "VpcReady" | ||
|
|
||
| // AWSClusterVpcReadyReason surfaces when the VPC for an AWSCluster is ready. | ||
| AWSClusterVpcReadyReason = clusterv1beta1.ReadyV1Beta2Reason | ||
|
|
||
| // AWSClusterVpcNotReadyReason surfaces when the VPC for an AWSCluster is not ready. | ||
| AWSClusterVpcNotReadyReason = clusterv1beta1.NotReadyV1Beta2Reason | ||
|
|
||
| // AWSClusterVpcDeletingReason surfaces when the VPC for an AWSCluster is being deleted. | ||
| AWSClusterVpcDeletingReason = clusterv1beta1.DeletingV1Beta2Reason | ||
| ) | ||
|
|
||
| // AWSCluster's SubnetsReady condition and corresponding reasons that will be used in v1Beta3 API version. | ||
| const ( | ||
| // AWSClusterSubnetsReadyCondition documents the status of subnets for an AWSCluster. | ||
| AWSClusterSubnetsReadyCondition = "SubnetsReady" | ||
|
|
||
| // AWSClusterSubnetsReadyReason surfaces when the subnets for an AWSCluster are ready. | ||
| AWSClusterSubnetsReadyReason = clusterv1beta1.ReadyV1Beta2Reason | ||
|
|
||
| // AWSClusterSubnetsNotReadyReason surfaces when the subnets for an AWSCluster are not ready. | ||
| AWSClusterSubnetsNotReadyReason = clusterv1beta1.NotReadyV1Beta2Reason | ||
|
|
||
| // AWSClusterSubnetsDeletingReason surfaces when the subnets for an AWSCluster are being deleted. | ||
| AWSClusterSubnetsDeletingReason = clusterv1beta1.DeletingV1Beta2Reason | ||
| ) | ||
|
|
||
| // AWSCluster's LoadBalancerReady condition and corresponding reasons that will be used in v1Beta3 API version. | ||
| const ( | ||
| // AWSClusterLoadBalancerReadyCondition documents the status of the load balancer for an AWSCluster. | ||
| AWSClusterLoadBalancerReadyCondition = "LoadBalancerReady" | ||
|
|
||
| // AWSClusterLoadBalancerReadyReason surfaces when the load balancer for an AWSCluster is ready. | ||
| AWSClusterLoadBalancerReadyReason = clusterv1beta1.ReadyV1Beta2Reason | ||
|
|
||
| // AWSClusterLoadBalancerNotReadyReason surfaces when the load balancer for an AWSCluster is not ready. | ||
| AWSClusterLoadBalancerNotReadyReason = clusterv1beta1.NotReadyV1Beta2Reason | ||
|
|
||
| // AWSClusterLoadBalancerDeletingReason surfaces when the load balancer for an AWSCluster is being deleted. | ||
| AWSClusterLoadBalancerDeletingReason = clusterv1beta1.DeletingV1Beta2Reason | ||
| ) | ||
|
|
||
| // AWSCluster's ClusterSecurityGroupsReady condition and corresponding reasons that will be used in v1Beta3 API version. | ||
| const ( | ||
| // AWSClusterSecurityGroupsReadyCondition documents the status of security groups for an AWSCluster. | ||
| AWSClusterSecurityGroupsReadyCondition = "ClusterSecurityGroupsReady" | ||
|
|
||
| // AWSClusterSecurityGroupsReadyReason surfaces when the security groups for an AWSCluster are ready. | ||
| AWSClusterSecurityGroupsReadyReason = clusterv1beta1.ReadyV1Beta2Reason | ||
|
|
||
| // AWSClusterSecurityGroupsNotReadyReason surfaces when the security groups for an AWSCluster are not ready. | ||
| AWSClusterSecurityGroupsNotReadyReason = clusterv1beta1.NotReadyV1Beta2Reason | ||
|
|
||
| // AWSClusterSecurityGroupsDeletingReason surfaces when the security groups for an AWSCluster are being deleted. | ||
| AWSClusterSecurityGroupsDeletingReason = clusterv1beta1.DeletingV1Beta2Reason | ||
| ) | ||
|
|
||
| // AWSCluster's BastionHostReady condition and corresponding reasons that will be used in v1Beta3 API version. | ||
| const ( | ||
| // AWSClusterBastionHostReadyCondition documents the status of the bastion host for an AWSCluster. | ||
| AWSClusterBastionHostReadyCondition = "BastionHostReady" | ||
|
|
||
| // AWSClusterBastionHostReadyReason surfaces when the bastion host for an AWSCluster is ready. | ||
| AWSClusterBastionHostReadyReason = clusterv1beta1.ReadyV1Beta2Reason | ||
|
|
||
| // AWSClusterBastionHostNotReadyReason surfaces when the bastion host for an AWSCluster is not ready. | ||
| AWSClusterBastionHostNotReadyReason = clusterv1beta1.NotReadyV1Beta2Reason | ||
|
|
||
| // AWSClusterBastionHostDeletingReason surfaces when the bastion host for an AWSCluster is being deleted. | ||
| AWSClusterBastionHostDeletingReason = clusterv1beta1.DeletingV1Beta2Reason | ||
| ) | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 don't think we should touch v1beta1 types at all.
Only v1beta2 ones
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.
@damdo just to make sure i’m understanding correctly about the
v1beta1conversion changes, should i remove theV1Beta3restoration logic fromawscluster_conversion.go? i followed the same pattern used for existing fields likeControlPlaneLoadBalancer, but i’m totally fine removing it if that’s not the direction we want to take.wanted to confirm this part before i move on to the rest of the feedback. thanks
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 we are adding new fields in v1beta2 then we need to preserve roundtrip compatibility. So it will require that you update the conversions in v1beta1 like you have done here to restore the value.
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.
A lot of people make the mistake of adding the new field to old api versions as well to make the conversions work. But restoring the value from the saved state is the correct way (like you are doing).
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.
Good point! Hadn't thought about that!