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.

Sign up for GitHub

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

Jump to bottom

Properly check condition array length for GKE status before accessing it #11213

Merged
merged 1 commit into from Oct 18, 2022
Merged

Properly check condition array length for GKE status before accessing it #11213

merged 1 commit into from Oct 18, 2022

Conversation

embik
Copy link
Member

@embik embik commented Oct 18, 2022 •
edited

What this PR does / why we need it:

Currently, the master-controller-manager is failing in dev with an index out of bounds error for this code. That is not surprising given that the if condition did not properly check if there are at least two conditions before attempting to index the second element.

I don't know if this is the proper fix since I'm not sure how the conditions on these API resources work (if there's only one condition, should it be ignored? is there something specific about the second element condition?), but at least this code should no longer result in an out-of-bounds panic.

Which issue(s) this PR fixes:

Fixes #

What type of PR is this?
/kind bug

Special notes for your reviewer:

Please let @imharshita review this given she knows the context of this code better.

Does this PR introduce a user-facing change? Then add your Release Note here:

Prevent index out-of-bounds issue when querying GKE external cluster status

Documentation:

NONE

@embik embik requested a review from imharshita October 18, 2022 06:44
@kubermatic-bot kubermatic-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. docs/none Denotes a PR that doesn't need documentation (changes). kind/bug Categorizes issue or PR as related to a bug. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. sig/cluster-management Denotes a PR or issue as being assigned to SIG Cluster Management. labels Oct 18, 2022
@kubermatic-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: embik

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

@kubermatic-bot kubermatic-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 18, 2022
@embik embik added backport-needed Denotes a PR or issue that has not been fully backported. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 18, 2022
@kubermatic-bot kubermatic-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 18, 2022
@embik embik added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 18, 2022
@embik
Properly check condition array length before accessing it
fd92025 
Signed-off-by: Marvin Beckers <marvin@kubermatic.com>
@embik embik changed the title Use index 0 instead of 1 when reading conditions for GKE status Properly check condition array length for GKE status before accessing it Oct 18, 2022
@embik
Copy link
Member Author

embik commented Oct 18, 2022

/cherry-pick release/v2.21

@kubermatic-bot
Copy link
Contributor

@embik: once the present PR merges, I will cherry-pick it on top of release/v2.21 in a new PR and assign it to you.

In response to this:

/cherry-pick release/v2.21

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.

@embik
Copy link
Member Author

embik commented Oct 18, 2022

/retest

1 similar comment
@embik
Copy link
Member Author

embik commented Oct 18, 2022

/retest

@wurbanski
Copy link
Contributor

/lgtm

@kubermatic-bot kubermatic-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 18, 2022
@kubermatic-bot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 738ce1cd6eae6d49a52a4ff996cad9b3d861b9a9

@kubermatic-bot kubermatic-bot merged commit 2f8eff7 into kubermatic:main Oct 18, 2022
@kubermatic-bot
Copy link
Contributor

@embik: new pull request created: #11215

In response to this:

/cherry-pick release/v2.21

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.

@kubermatic-bot kubermatic-bot mentioned this pull request Oct 18, 2022
Merged
@embik embik deleted the oob-gke-fix branch October 18, 2022 15:20
@embik embik added backport-complete Denotes a PR or issue which has been fully backported to all required release branches. and removed backport-needed Denotes a PR or issue that has not been fully backported. labels Oct 24, 2022
xrstf added a commit to xrstf/kubermatic-dashboard that referenced this pull request Oct 28, 2022
@xrstf
apply: Properly check condition array length before accessing it (kub…
17266b5 
…ermatic/kubermatic#11213)
kubermatic-bot pushed a commit to kubermatic/dashboard that referenced this pull request Oct 29, 2022
@xrstf
bump KKP to 48b7e886aed9 (#5165)
5996cd7 
* bump KKP to 48b7e886aed9

* apply: Update tests to kind 0.16 (kubermatic/kubermatic#11133)

* apply: Properly check condition array length before accessing it (kubermatic/kubermatic#11213)

* apply: Switch AWS account for E2E tests (kubermatic/kubermatic#11211)

* apply: Refactor KubeVirt cloud provider - part 1 (kubermatic/kubermatic#11202)

* apply: Add Kubernetes 1.25 (kubermatic/kubermatic#11049)

* apply: Add feature flag to enable/disable the Web Terminal for the user clusters (kubermatic/kubermatic#11243)

* apply: Fix panic in cluster webhook (kubermatic/kubermatic#11236)

* apply: Cleanup tests, improve logging (kubermatic/kubermatic#11228)

* apply: Configure machine flavor filters at datacenter level (kubermatic/kubermatic#11130)

* apply: update KubevirtNodeSpec with topologyConstraint (kubermatic/kubermatic#11114)

* update swagger spec / apiclient
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imharshita imharshita Awaiting requested review from imharshita

Assignees

@wurbanski wurbanski

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. backport-complete Denotes a PR or issue which has been fully backported to all required release branches. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. docs/none Denotes a PR that doesn't need documentation (changes). kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-management Denotes a PR or issue as being assigned to SIG Cluster Management. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@embik @kubermatic-bot @wurbanski