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

OCPBUGS-14862 Improve clarity around hypershift operator permissions #2782

Merged
merged 1 commit into from Jul 12, 2023
Merged

OCPBUGS-14862 Improve clarity around hypershift operator permissions #2782

merged 1 commit into from Jul 12, 2023

Conversation

mjlshen
Copy link
Contributor

@mjlshen mjlshen commented Jul 8, 2023 •
edited

  • The operator now expects to be able to perform ec2:RejectVpcEndpointConnections
  • The message of AWS errors is logged, which can help identify which AWS operation is running into errors.

What this PR does / why we need it:
In response to feedback during QA, we should document required permissions and return AWS error messages to improve the troubleshooting experience when AWS permissions are missing.

Which issue(s) this PR fixes:
OCPBUGS-14862

Checklist

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.
  • This change includes docs.
  • This change includes unit tests.

@openshift-ci openshift-ci bot requested review from enxebre and sjenning July 8, 2023 23:10
@openshift-ci openshift-ci bot added area/documentation Indicates the PR includes changes for documentation area/hypershift-operator Indicates the PR includes changes for the hypershift operator and API - outside an OCP release and removed do-not-merge/needs-area labels Jul 8, 2023
@netlify
Copy link

netlify bot commented Jul 8, 2023

✅ Deploy Preview for hypershift-docs ready!

Name Link
🔨 Latest commit 7976ad6
🔍 Latest deploy log https://app.netlify.com/sites/hypershift-docs/deploys/64a9ecba9cdf260008c2d062
😎 Deploy Preview https://deploy-preview-2782--hypershift-docs.netlify.app/how-to/aws/deploy-aws-private-clusters
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mjlshen
Copy link
Contributor Author

mjlshen commented Jul 9, 2023

/retest

@mjlshen
Improve clarity around hypershift operator permissions
961491d 
* The operator now expects to be able to perform
  ec2:RejectVpcEndpointConnections
* The message of AWS errors is logged, which can help identify which
  AWS operation is running into errors.

Signed-off-by: Michael Shen <mshen@redhat.com>
mjlshen
mjlshen commented Jul 10, 2023
View reviewed changes
hypershift-operator/controllers/platform/aws/controller.go
if awsErr, ok := err.(awserr.Error); ok {
log.Info("AWS Error", "code", awsErr.Code(), "message", awsErr.Message())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Logging the error message instead of returning it because of feedback I received:

the reason we don't include the error message in the error we return is that AWS usually puts something in there that changes every time. And we update the HostedCluster status with the text from the error we return. That means that if we're constantly getting an error, we're going to be updating the HC status many times, which is not a good thing.

cc @csrwng

@mjlshen
Copy link
Contributor Author

mjlshen commented Jul 10, 2023

/retest

@csrwng
Copy link
Contributor

csrwng commented Jul 11, 2023

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 11, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: csrwng, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 11, 2023
@muraee
Copy link
Contributor

muraee commented Jul 11, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2023
@csrwng
Copy link
Contributor

csrwng commented Jul 11, 2023

/cherry-pick release-4.13

@openshift-cherrypick-robot
Copy link

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

In response to this:

/cherry-pick release-4.13

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.

@mjlshen
Copy link
Contributor Author

mjlshen commented Jul 11, 2023

/test e2e-kubevirt-aws-ovn

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 9776a1f and 2 for PR HEAD 961491d in total

@mjlshen
Copy link
Contributor Author

mjlshen commented Jul 11, 2023

/retest

3 similar comments
@mjlshen
Copy link
Contributor Author

mjlshen commented Jul 12, 2023

/retest

@mjlshen
Copy link
Contributor Author

mjlshen commented Jul 12, 2023

/retest

@mjlshen
Copy link
Contributor Author

mjlshen commented Jul 12, 2023

/retest

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD bf1f476 and 1 for PR HEAD 961491d in total

@mjlshen
Copy link
Contributor Author

mjlshen commented Jul 12, 2023

/retest

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 12, 2023

@mjlshen: 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/test-infra repository. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit 0823802 into openshift:main Jul 12, 2023
12 checks passed
@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Jul 12, 2023
Merged
@openshift-cherrypick-robot
Copy link

@csrwng: new pull request created: #2810

In response to this:

/cherry-pick release-4.13

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.

@mjlshen mjlshen deleted the OCPBUGS-14862 branch July 13, 2023 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@enxebre enxebre Awaiting requested review from enxebre

@sjenning sjenning Awaiting requested review from sjenning

Assignees

@muraee muraee

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/documentation Indicates the PR includes changes for documentation area/hypershift-operator Indicates the PR includes changes for the hypershift operator and API - outside an OCP release lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@mjlshen @csrwng @muraee @openshift-cherrypick-robot @openshift-ci-robot @openshift-merge-robot