-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add credentials secret to AWS provider config in order to support CredentialsRequest #1281
Add credentials secret to AWS provider config in order to support CredentialsRequest #1281
Conversation
/hold |
If we do this by setting a secret on every MachineSet, and ultimately remove the IAM role that allows the machine API to work today, the default state for anyone who creates a MachineSet would be broken? Just wondering if this would be best done by default in the machine-api, rather than having it pinned on every MachineSet. |
Any defaults would have to go into the controllers in our fork of the cluster-api, and further deviate from upstream. Users already have to choose quite a few things in the We probably want to start using |
I understand, thanks! |
/retest |
This isn't going to pass until openshift/machine-api-operator#199 is merged. |
@bison you sure, because they still have the same thing in their repository: |
@spangenberg, ah, I forgot about that. That is using the wrong namespace though. We changed it to |
/test e2e-aws openshift/machine-api-operator#199 got merged 🎉 |
/retest |
1 similar comment
/retest |
/hold cancel |
/retest |
1 similar comment
/retest |
can we then get rid of https://github.com/openshift/installer/blob/master/data/data/aws/master/main.tf#L36 as part of this PR? cc @dgoodwin @abhinavdahiya @spangenberg |
Hopefully we'd be at a point where we can try at least, I think that was an end goal of all this. |
Machine controller is throwing errors:
|
some perm must me missed, you should be able to use |
This should be unblocked by openshift/cloud-credential-operator#37 and additionally openshift/machine-api-operator#241. |
Depending PRs are all merged now. /retest |
34d339b
to
ab0e2e2
Compare
data/data/aws/master/main.tf
Outdated
@@ -33,41 +29,6 @@ EOF | |||
), var.tags)}" | |||
} | |||
|
|||
resource "aws_iam_role_policy" "master_policy" { |
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.
We need the iam policy for kubelet to running. also the kube-controller-manager would need access to AWS api too.
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.
I removed the change, something like this can happen in another PR if wanted.
ab0e2e2
to
30f5cf0
Compare
/retest |
/cc @wking @abhinavdahiya |
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: abhinavdahiya, spangenberg 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 |
/retest Please review the full test history for this PR and help us cut down flakes. |
/cc bison