Unable to assume CDK lookup role using temporary session credentials #62
Comments
tranhl
changed the title
|
Turns out the issue is that |
|
@tranhl sorry you ran into this error, we should probably update the readme to note that you have to specify the |
|
@corymhall I think I stumbled on this as well but I'm not sure that I agree with the solution. All my stacks are working fine on the local console using AWS_PROFILE= and AWS SSO temporary credentials, the This is in-line with the actual documentation of the behaviour from CDK - example here. I do not need/want to make my stacks non-account agnostic. Github Action Then in my GH action setup, I do have an IAM Role that is successfully assumed via OIDC: and I have granted this role the CloudFormationReadOnlyAccess AWS Managed policy that should be enough to perform the diff. However the action is trying to assume the lookup role instead of the temporary credentials obtained via OIDC: I would say the logic here should be exactly the opposite, use the temporary credentials if they are present (or maybe that could be a flag?) EDIT: Before running this action, I tried to run I'll keep investigating why this does work on a local CLI with agnostic stacks but doesn't work with temporary credentials on the handcrafted IAM role. |
|
@rantoniuk this guide here has some additional details about how the CDK gets credentials for things. The CDK CLI (and this action) will only use your credentials to assume-role into the role that actually has the permissions to perform the action. https://github.com/aws/aws-cdk/wiki/Security-And-Safety-Dev-Guide#defaultstacksynthesizer |
|
I have fixed the problem of the manual to the role that is assumed in the OIDC step: and now I see exactly the same output as when I'm doing a diff locally on the CLI console. Yet, in the next step, this action still has the same issue as before, trying to assume a role without expanding the AccountID placeholder: I'm going to try to build a custom version of the action with the swapped logic to see if that fixes the issue. |
|
@corymhall the action works flawlessly when I removed the custom credentials logic and disabled associated tests and tested via In the commented PR I see wrong warning:
that is also coming from the action logic. Take a look at the changes and let me know if you think we could incorporate them into your action. I think that should be the default behaviour but I'd be also happy with a flag. Happy to provide a PR! |
|
@rantoniuk thanks for digging into this, I think I'm finally seeing what the issue is. There are two cases that are hard to tell apart and we currently only support one of them
In the first case the user has to provide the environment otherwise there is no way to tell which environment to perform the diff for. We just need to handle the second case. When we get the credentials here: Lines 75 to 87 in 94b25b3 If the user provides the lookup role and the arn has an unknown account/region we should just substitute the current account/region (from the current credentials) like we do for the partition. We should also still log a warning to let the user know that we are making this assumption because there is a chance that they should have provided the environment, but have a misconfiguration. I won't be able to pick this one up in the short term, but would definitely take a PR! |
corymhall
added
bug
|
@corymhall I agree about the unsupported case, but I fail to understand why would this action need to support anything at all instead of relying on the credentials provided in earlier pipeline steps. The purpose of this action could/should be just to interpret the diff based on Whether the stack is environment-agnostic or not, is not to be decided by this action, but instead by the Stack configuration. The reason my version works is because:
to assume the corresponding lookup roles in the target account that are defined in the In other words, responding to your comment - I'm not providing any lookup roles explicitly, everything is handled by CDK itself. For the 1) supported case My underlying logic here is: as long as |
I think we are saying similar things. This action is trying to mimic the behavior of the CDK CLI. When you run This action attempts to do the same thing. When it performs the diff it reads the CloudAssembly and if you are using the The reason you are running into this error in this action, but not when using the CLI is because the CLI just has more robust credential logic and this action suffers from the bug I mentioned in my last comment. |
Is there any specific reason it does that instead of relying on CDK buil-in capabilities? After all, you anyway are running
Again, from my perspective the bug is that it implements the custom logic in the first place, because when this is removed, everything works fine with the role that was assumed in the previous step and to which the short-lived credentials are present. |
|
@rantoniuk the custom logic exists because I want more control over the output. Because of the custom logic this action is able to do things like highlight resources that have destructive changes, fail the workflow if there are destructive changes, filter on what resources to allow destructive changes on, etc. If all you want is the raw diff output from |
|
Unfortunately this does not answer my question at all. What you're describing is post-authentication logic and this whole discussion is about why the custom logic exists for authentication: Lines 72 to 83 in 773b5ec I think I explained my view above already and I don't want to pollute the issue. From my POV if you are fine with adding a flag to use the default credentials, then I'm happy to do it, but I don't see any point of implementing yet more complex custom logic to cover the use case in the action itself, because it shouldn't be here imo. |
|
When you perform
There is no ability to pick and choose which things you want and which you do not. In my case for this action, I want to change the very last thing. I want to take the output of This is probably more information than you care for, but it's why I don't want to add the flag that you are suggesting. Anyone should definitely feel free to fork this action and customize it to work better for you individual use case. |
Having a bit of trouble getting this action to work. Using the following configuration:
I get the following error:
Seems like the issue is that
AWS::AccountIdandAWS::Regionisn't templating correctly when assuming the CDK lookup role? Not exactly sure why that would be the case. I've included the full error logs, happy to provide additional information needed.Full error log
The text was updated successfully, but these errors were encountered: