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

Bug 1981550: aws: move elastic ip permissions to create networking category #5045

Merged

Conversation

patrickdillon
Copy link
Contributor

Elastic IPs are only created when the installer provisions VPCs and subnets. In the case where users install into an existing VPC, the installer does not need the permissions to create and associate EIPs. Therefore let's move these EIP permissions so that they are only checked when we create networking resources.

@openshift-ci openshift-ci bot requested review from e-tienne and jstuever June 30, 2021 14:23
@patrickdillon
Copy link
Contributor Author

/hold

This behavior has not been confirmed. @yunjiang29 has offered to confirm this behavior.

To confirm this behavior:

  • remove these permissions from the installer user: ec2:AllocateAddress, ec2:AssociateAddress, ec2:ReleaseAddress
  • use the installer built from this PR to install a cluster into an existing network
  • delete the cluster

If the install succeeds, we have a bug and we can create a BZ/merge this PR.

If the install fails, please give the logs so we can determine which resource needs the permission. In this case it might be helpful to test a publish:internal cluster as well.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 30, 2021
@yunjiang29
Copy link
Contributor

@patrickdillon installation failed with fatal error:

level=warning msg=Action not allowed with tested creds action=ec2:ReleaseAddress
level=warning msg=Tested creds not able to perform all requested actions
level=fatal msg=failed to fetch Cluster: failed to fetch dependency of "Cluster": failed to generate asset "Platform Permissions Check": validate AWS credentials: current credentials insufficient for performing cluster installation


logs and infos

  • AWS user policy
{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Sid": "VisualEditor0",
            "Effect": "Deny",
            "Action": [
                "ec2:AllocateAddress",
                "ec2:AssociateAddress",
                "ec2:ReleaseAddress"
            ],
            "Resource": "*"
        },
        {
            "Effect": "Allow",
            "Action": "*",
            "Resource": "*"
        }
    ]
}
  • cluster-bot:
build 4.8.0-rc.1,openshift/installer#5045

image build log: https://prow.ci.openshift.org/view/gs/origin-ci-test/logs/release-openshift-origin-installer-launch-gcp/1410416732882866176

  • install log:
level=info msg=Credentials loaded from the "pr5045" profile in file "/home/cloud-user/.aws/credentials"
level=info msg=Consuming Install Config from target directory
level=warning msg=Action not allowed with tested creds action=ec2:ReleaseAddress
level=warning msg=Tested creds not able to perform all requested actions
level=fatal msg=failed to fetch Cluster: failed to fetch dependency of "Cluster": failed to generate asset "Platform Permissions Check": validate AWS credentials: current credentials insufficient for performing cluster installation
  • install-config:
apiVersion: v1
baseDomain: qe.devcluster.openshift.com
compute:
- architecture: amd64
  hyperthreading: Enabled
  name: worker
  platform: {}
  replicas: 3
controlPlane:
  architecture: amd64
  hyperthreading: Enabled
  name: master
  platform: {}
  replicas: 3
metadata:
  creationTimestamp: null
  name: yunjiang-pr45a
networking:
  clusterNetwork:
  - cidr: 10.128.0.0/14
    hostPrefix: 23
  machineNetwork:
  - cidr: 10.0.0.0/16
  networkType: OpenShiftSDN
  serviceNetwork:
  - 172.30.0.0/16
platform:
  aws:
    region: us-east-2
    subnets:
    - subnet-00016dae79e9a8023
    - subnet-077cc0c20a44ca9e4
publish: Internal
pullSecret: HIDDEN
sshKey: HIDDEN

Elastic IPs are only created when the installer provisions VPCs and
subnets. In the case where users install into an existing VPC, the
installer does not need the permissions to create and associate EIPs.
Therefore let's move these EIP permissions so that they are only checked
when we create networking resources.
@patrickdillon
Copy link
Contributor Author

@patrickdillon installation failed with fatal error:

@yunjiang29 thanks for testing. There was a mistake in the PR (ec2:ReleaseAddress was in the wrong group). Would you mind testing again when you have a moment?

@yunjiang29
Copy link
Contributor

Hello @patrickdillon , tests get passed, see my test steps below:

steps

  • create a IAM user with following policy, and set as default user for installer
{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Sid": "VisualEditor0",
            "Effect": "Deny",
            "Action": [
                "ec2:AllocateAddress",
                "ec2:AssociateAddress",
                "ec2:ReleaseAddress"
            ],
            "Resource": "*"
        },
        {
            "Effect": "Allow",
            "Action": "*",
            "Resource": "*"
        }
    ]
}
  • install private cluster publish: Internal into an existing VPC (SUCCEEDED, PASS)
level=info msg=Credentials loaded from the "pr5045" profile in file "/home/cloud-user/.aws/credentials"
level=info msg=Consuming Install Config from target directory
level=info msg=Creating infrastructure resources...
<--snip-->
level=info msg=Time elapsed: 32m59s
  • Destroy cluster (SUCCEEDED, PASS)
level=info msg=Credentials loaded from the "pr5045" profile in file "/home/cloud-user/.aws/credentials"
<--snip-->
level=info msg=Time elapsed: 6m14s
  • install a public cluster publish: External, did not provide existing subnets. (FAILED as expected, PASS)
level=info msg=Credentials loaded from the "pr5045" profile in file "/home/cloud-user/.aws/credentials"
level=info msg=Consuming Install Config from target directory
level=warning msg=Action not allowed with tested creds action=ec2:AllocateAddress
level=warning msg=Action not allowed with tested creds action=ec2:AssociateAddress
level=warning msg=Action not allowed with tested creds action=ec2:ReleaseAddress
level=warning msg=Tested creds not able to perform all requested actions
level=fatal msg=failed to fetch Cluster: failed to fetch dependency of "Cluster": failed to generate asset "Platform Permissions Check": validate AWS credentials: current credentials insufficient for performing cluster installation

@patrickdillon
Copy link
Contributor Author

/hold cancel
Thank you @yunjiang29

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 2, 2021
@patrickdillon
Copy link
Contributor Author

/cherry-pick release-4.8
/cherry-pick release-4.7

@openshift-cherrypick-robot

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

In response to this:

/cherry-pick release-4.8
/cherry-pick release-4.7

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.

@patrickdillon
Copy link
Contributor Author

/cherry-pick release-4.7

@openshift-cherrypick-robot

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

In response to this:

/cherry-pick release-4.7

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.

@jhixson74
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 2, 2021
@jhixson74
Copy link
Member

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 2, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhixson74

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 2, 2021
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

8 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 3, 2021

@patrickdillon: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-crc f03cc20 link /test e2e-crc
ci/prow/e2e-aws-workers-rhel7 f03cc20 link /test e2e-aws-workers-rhel7

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-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-cherrypick-robot

@patrickdillon: new pull request created: #5055

In response to this:

/cherry-pick release-4.7

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-cherrypick-robot

@patrickdillon: new pull request created: #5056

In response to this:

/cherry-pick release-4.8
/cherry-pick release-4.7

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.

@patrickdillon patrickdillon changed the title aws: move elastic ip permissions to create networking category Bug 1981550: aws: move elastic ip permissions to create networking category Jul 12, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 12, 2021

@patrickdillon: All pull requests linked via external trackers have merged:

Bugzilla bug 1981550 has been moved to the MODIFIED state.

In response to this:

Bug 1981550: aws: move elastic ip permissions to create networking category

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. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants