-
Notifications
You must be signed in to change notification settings - Fork 111
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
Update filter logic for finding subnets and security groups due to 4.16 changes #596
Update filter logic for finding subnets and security groups due to 4.16 changes #596
Conversation
ec9fb54
to
468a41a
Compare
{ | ||
Name: aws.String(fmt.Sprintf("tag:kubernetes.io/cluster/%s", e.cluster.InfraID())), | ||
Values: []string{"owned"}, | ||
Values: []string{fmt.Sprintf("%s-master-sg", e.cluster.InfraID()), fmt.Sprintf("%s-controlplane", e.cluster.InfraID())}, |
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 4.16 <infra_id>-master-sg
=> <infra_id>-controlplane
. We need to check for both now.
/hold |
468a41a
to
beaf5a3
Compare
/unhold |
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.
Minor suggestion for some comments, but otherwise LGTM. Feel free to either add, or not, the comments, and I'll LGTM officially either way.
/label tide/merge-method-squash |
d92776e
to
2b697bb
Compare
…16 changes. Co-authored-by: Christopher Collins <collins.christopher@gmail.com> Co-authored-by: Michael Shen <mishen@umich.edu>
bdb12f0
to
df89fb2
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AlexVulaj, mjlshen 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 |
@AlexVulaj: 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. |
Resolves OSD-24457.
As of 4.16, some downstream component is causing the subnet tag
kubernetes.io/cluster/<infra_id>
to show as "shared" instead of "owned" now, even for fresh installs in clean AWS accounts. As far as the network verifier is concerned, we probably don't actually care about the value of this tag, just that it exists.There was also a regression captured in OCPBUGS-36902 with the
kubernetes.io/cluster/<infra_id>
tag being removed from security groups. Since theName
tag already includes a unique cluster identifier, I've dropped this tag entirely from the filters to prevent issues with searching.I verified on a cluster installed at 4.1.z that the Name tag was present with the value that we expect, so I wouldn't expect any issues with this change on older clusters.