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

(pipelines): SelfMutate loads notices by default #19542

Closed
Kruspe opened this issue Mar 24, 2022 · 7 comments · Fixed by #19967
Closed

(pipelines): SelfMutate loads notices by default #19542

Kruspe opened this issue Mar 24, 2022 · 7 comments · Fixed by #19967
Assignees
Labels
@aws-cdk/pipelines CDK Pipelines library bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. p2

Comments

@Kruspe
Copy link
Contributor

Kruspe commented Mar 24, 2022

What is the problem?

When using the SelfMutate step it calls cdk -a ${toPosixPath(embeddedAsmPath(this.pipeline))} deploy ${pipelineStackIdentifier} --require-approval=never --verbose. With the new cdk release this fetches notices. If the SelfMutate runs in a private subnet without internet connection the step takes really long until the request times out. (Build times go up from 2 minutes to 16 minutes).

Reproduction Steps

Create a pipeline which uses the SelfMutate option and runs in a private subnet.

What did you expect to happen?

For the SelfMutate I do not think that it is necessary to fetch the notices.

What actually happened?

The deploy step tries to fetch the notices.

CDK CLI Version

2.17.0

Node.js Version

16.14.0

OS

MacOS Monetery 12.3

Language

Typescript

I think it would suffice to add the --no-notices flag to the cdk deploy command.

@Kruspe Kruspe added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 24, 2022
@github-actions github-actions bot added the @aws-cdk/pipelines CDK Pipelines library label Mar 24, 2022
@ryparker ryparker added the p2 label Mar 25, 2022
@Kruspe
Copy link
Contributor Author

Kruspe commented Mar 26, 2022

If you agree with my assumptions I can provide a PR to fix this.

@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 28, 2022

Unfortunately adding --no-notices would break everybody deliberately using an old version of the CLI.

What version of the CLI are you using? We've reduced the timeout so the delay should not be noticeable anymore.

@rix0rrr rix0rrr added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Mar 28, 2022
@Kruspe
Copy link
Contributor Author

Kruspe commented Mar 28, 2022

Yeah I see that problem. What we could do is a version check.

We are not providing a cliVersion so we are using the latest version (2.17.0). I saw the PR but it does not seem to do what it is suppose to do. 🤔

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Mar 28, 2022
@otaviomacedo
Copy link
Contributor

otaviomacedo commented Mar 31, 2022

Hi, @Kruspe.

How does npm install the latest version when it doesn't have access to the internet? Are you using CodeArtifact? Can you please confirm the version by looking at the debug output? It's the first line.

@Kruspe
Copy link
Contributor Author

Kruspe commented Mar 31, 2022

We are using a proxy to download the necessary things and are providing a list of whitelisted urls. NPM is Part of it but the notices endpoint is not.
The version being downloaded is the latest one. It was 2.17.0 and now it is 2.18.0.

@Kruspe
Copy link
Contributor Author

Kruspe commented Apr 5, 2022

@rix0rrr & @otaviomacedo any thoughts on this?

rix0rrr added a commit that referenced this issue Apr 19, 2022
In an environment where DNS resolution works but network packets may be
dropped, the CLI can hang for up to 15 minutes trying to refresh
notices. We tried to set a timeout of 3 seconds on this, but did it in a
way that didn't work.

Turns out that the `request.on('timeout')` event in NodeJS doesn't actually stop
the request: it just notifies the listener that it's been a long time
since we saw network bytes, leaving the listener to do with that
what they want (potentially show a dialog box to a waiting user or
whatever). In our case, we had to do an additional `request.destroy()`
to actually stop the request.

Without doing this, the Promise would resolve correctly and the CLI
would continue, but the actual HTTP request would still be going on
in the background, preventing Node from shutting down.

This PR also changes the behavior of reporting a download failure: it
used to be that we would successfully resolve to a `[]` response if
the HTTP request failed (which would then be cached). Instead, after
this change we reject the Promise if anything goes wrong, so that the
next invocation will try again.

Fixes #19542.
@mergify mergify bot closed this as completed in #19967 Apr 19, 2022
mergify bot pushed a commit that referenced this issue Apr 19, 2022
In an environment where DNS resolution works but network packets may be
dropped, the CLI can hang for up to 15 minutes trying to refresh
notices. We tried to set a timeout of 3 seconds on this, but did it in a
way that didn't work.

Turns out that the `request.on('timeout')` event in NodeJS doesn't actually stop
the request: it just notifies the listener that it's been a long time
since we saw network bytes, leaving the listener to do with that
what they want (potentially show a dialog box to a waiting user or
whatever). In our case, we had to do an additional `request.destroy()`
to actually stop the request.

Without doing this, the Promise would resolve correctly and the CLI
would continue, but the actual HTTP request would still be going on
in the background, preventing Node from shutting down.

This PR also changes the behavior of reporting a download failure: it
used to be that we would successfully resolve to a `[]` response if
the HTTP request failed (which would then be cached). Instead, after
this change we reject the Promise if anything goes wrong, so that the
next invocation will try again.

Fixes #19542.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

StevePotter pushed a commit to StevePotter/aws-cdk that referenced this issue Apr 27, 2022
In an environment where DNS resolution works but network packets may be
dropped, the CLI can hang for up to 15 minutes trying to refresh
notices. We tried to set a timeout of 3 seconds on this, but did it in a
way that didn't work.

Turns out that the `request.on('timeout')` event in NodeJS doesn't actually stop
the request: it just notifies the listener that it's been a long time
since we saw network bytes, leaving the listener to do with that
what they want (potentially show a dialog box to a waiting user or
whatever). In our case, we had to do an additional `request.destroy()`
to actually stop the request.

Without doing this, the Promise would resolve correctly and the CLI
would continue, but the actual HTTP request would still be going on
in the background, preventing Node from shutting down.

This PR also changes the behavior of reporting a download failure: it
used to be that we would successfully resolve to a `[]` response if
the HTTP request failed (which would then be cached). Instead, after
this change we reject the Promise if anything goes wrong, so that the
next invocation will try again.

Fixes aws#19542.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/pipelines CDK Pipelines library bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants