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.

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

OCPBUGS-19757: bump controller iam policy to v2.4.7 #114

Merged
merged 3 commits into from
Oct 27, 2023

Conversation

alebedev87
Copy link
Contributor

@alebedev87 alebedev87 commented Oct 17, 2023

Context
AWS changed the required permissions to create a LB target group: upstream issue describing the change.

Why didn't we catch this in CI?
The problem started to be visible after AWS CLI commands were published. With these commands the upstream IAM policy started to be used instead of the generated controller's credentials request. The difference between the two is that the latter is minified to meet the AWS limit for inline policies. The minified CredentialsRequest has a broader set of permissions and since it's used in the CI to create the controller's role we never saw the missing permissions.

Changes
The main change is a bump of the upstream IAM policy (assets directory). However to catch similar problems in the future we need to have the controller's CredentialsRequest to be the same as the upstream's IAM policy. For this we need to stop minifying. This in turn requires significant changes in iamctl binary to be able to support:

  • multiple resources from AWS statements because CredentialsRequest's resource field is not a slice (-s flag added)
  • conditions from AWS statements
  • customization of the IAM Policy go function (-f flag)

Result
The IAM policy is bumped enabling the users of AWS CLI command to use the right set of permissions for the controller's role. The controller's CredentialsRequest is not minified anymore so that the role created in ROSA e2e using ccoctl is as similar as possible to the upstream.

Note
The IAM policy generated for non-STS clusters must still be minified as the AWS limit is still there. A new IAM policy for STS will be added in this PR to be used in case the STS role is given to the controller.

@openshift-ci-robot openshift-ci-robot added jira/severity-important Referenced Jira bug's severity is important for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Oct 17, 2023
@openshift-ci-robot
Copy link

@alebedev87: This pull request references Jira Issue OCPBUGS-19757, which is invalid:

  • expected the bug to target the "4.15.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Significant changes were required in iamctl to be able to support:

  • multiple resources from AWS statements (CredentialsRequest has a single resource API)
  • conditions from AWS statements

The controller's CredentialsRequest is not minified anymore so that the role created in ROSA e2e using ccoctl would be as similar as possible to the upstream. The bug was not caught by the CI mostly because of the minification which granted more rights than the upstream requires. However the minified version is still necessary for a non-STS cluster due to the AWS limitation of the length of the inline policies (2048 chars).

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.

@openshift-ci openshift-ci bot requested review from gcs278 and miheer October 17, 2023 10:53
@alebedev87
Copy link
Contributor Author

/test e2e-aws-rosa-operator

@alebedev87
Copy link
Contributor Author

/jira refresh

@openshift-ci-robot openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Oct 17, 2023
@openshift-ci-robot
Copy link

@alebedev87: This pull request references Jira Issue OCPBUGS-19757, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.15.0) matches configured target version for branch (4.15.0)
  • bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @ShudiLi

In response to this:

/jira refresh

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.

@openshift-ci openshift-ci bot requested a review from ShudiLi October 17, 2023 14:15
@openshift-ci-robot
Copy link

@alebedev87: This pull request references Jira Issue OCPBUGS-19757, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.15.0) matches configured target version for branch (4.15.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @ShudiLi

In response to this:

Significant changes were required in iamctl to be able to support:

  • multiple resources from AWS statements because CredentialsRequest's resource field is not a slice (-s flag added)
  • conditions from AWS statements
  • customization of the IAM Policy go function (-f flag)

The controller's CredentialsRequest is not minified anymore so that the role created in ROSA e2e using ccoctl would be as similar as possible to the upstream. The bug was not caught by the CI mostly because of the minification which granted more rights than the upstream requires. However the minified version is still necessary for a non-STS cluster due to the AWS limitation of the length of the inline policies (2048 chars).

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.

@alebedev87
Copy link
Contributor Author

/test e2e-aws-operator

@alebedev87
Copy link
Contributor Author

/test e2e-aws-proxy-operator

@alebedev87
Copy link
Contributor Author

/assign @thejasn

@thejasn
Copy link
Contributor

thejasn commented Oct 25, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2023
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 26, 2023
@alebedev87
Copy link
Contributor Author

/test e2e-aws-rosa-operator

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 26, 2023

@alebedev87: 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.

@thejasn
Copy link
Contributor

thejasn commented Oct 26, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 26, 2023
go fmt -mod=vendor $(IAMCTL_OUTPUT_DIR)/$(IAMCTL_OUTPUT_FILE)
go vet -mod=vendor $(IAMCTL_OUTPUT_DIR)/$(IAMCTL_OUTPUT_FILE)
# generate controller's IAM policy without minify.
@# This policy is for STS clusters as it's turned into a role policy which is limited to 10240 by AWS.
Copy link

Choose a reason for hiding this comment

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

Okay, so if I understand this correctly, you could minify for STS clusters as well, but by not minifying, you help expose issues with the non-minified IAM Policy, of which QE uses to install clusters and is documented Option 2. Using the AWS CLI in install.md?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly.

@gcs278
Copy link

gcs278 commented Oct 27, 2023

I just had a clarifying question, but thanks for the very detailed description. It took me couple of re-reads, but I think I understand your motivation.
/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 27, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gcs278

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 Oct 27, 2023
@openshift-ci openshift-ci bot merged commit 41b799d into openshift:main Oct 27, 2023
@openshift-ci-robot
Copy link

@alebedev87: Jira Issue OCPBUGS-19757: All pull requests linked via external trackers have merged:

Jira Issue OCPBUGS-19757 has been moved to the MODIFIED state.

In response to this:

Context
AWS changed the required permissions to create a LB target group: upstream issue describing the change.

Why didn't we catch this in CI?
The problem started to be visible after AWS CLI commands were published. With these commands the upstream IAM policy started to be used instead of the generated controller's credentials request. The difference between the two is that the latter is minified to meet the AWS limit for inline policies. The minified CredentialsRequest has a broader set of permissions and since it's used in the CI to create the controller's role we never saw the missing permissions.

Changes
The main change is a bump of the upstream IAM policy (assets directory). However to catch similar problems in the future we need to have the controller's CredentialsRequest to be the same as the upstream's IAM policy. For this we need to stop minifying. This in turn requires significant changes in iamctl binary to be able to support:

  • multiple resources from AWS statements because CredentialsRequest's resource field is not a slice (-s flag added)
  • conditions from AWS statements
  • customization of the IAM Policy go function (-f flag)

Result
The IAM policy is bumped enabling the users of AWS CLI command to use the right set of permissions for the controller's role. The controller's CredentialsRequest is not minified anymore so that the role created in ROSA e2e using ccoctl is as similar as possible to the upstream.

Note
The IAM policy generated for non-STS clusters must still be minified as the AWS limit is still there. A new IAM policy for STS will be added in this PR to be used in case the STS role is given to the controller.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/severity-important Referenced Jira bug's severity is important for the branch this PR is targeting. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants