[Backport] Google CloudBuild support (#9090) #9165
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Part of this change is implementing a "no secrets" policy for CI. Given that
we have to support CI for arbitrary external contributors, and
it is easy to craft a malicious PR that exfiltrates secrets during a CI build
any test that runs under CI must be able to do so without any injected secrets.
This means that several of the test we currently run under Drone will not be run on GCB, at least as part of the regular CI. The plan is to create a separate task that periodically runs tests that require external credentials (e.g. Kube tests, various backend data stores, etc.) in a more secure way and report failures asynchronously. And while these tests will not run under CI, the should still be built under CI so that required changes are caught during review.
tcsc
changed the title
russjones
approved these changes
Nov 30, 2021
The race condition detector is being tripped by a concurrent `Write` and `Close` in the `PipeNetCon` in several integration tests. This is a naive fix to serialize the write and close operations to resolve the race condition. The affected tests were also not handling asynchronous error reporting correctly (i.e. it's not legal to call `require.XYZ()` from a goroutine other than the one executing the test function.). This patch introduces some plumbing to marshal asynchronous errors back into the main test routine before failing the test.
Some integration tests modify global "constants" to speed up test execution (e.g. shortening polling intervals). This is occasionally tripping the Go data race detector, so I have added explicit serialisation to reading and writing these global settings. These values are only ever changed in a test environment, and there should be zero contention for them in a non-test environment.
When disconnecting a client due to an expired certificate, the monitor
was emitting the disconnection event before forcing the client to
disconnect. This led to races in tests that wait on the disconnection
event, where tests would:
1. wait for the disconnection event before proceeding
2. receive the disconnection event
3. take action based on the disconnection event, assuming that the
client was disconnected, when there is no guarantee that the
disconnection has actually happened yet.
This patch re-orders the disconnection and event broadcast, such that
the disconnection happens first, meaning that if a watcher receives
the disconnection event, then the disconnection has already been
attempted.
tcsc
force-pushed
the
tcsc/backport-v8/gcb
branch
from
dd08c25 to
d33bf5f
Compare
There is an intermittent issue where test will fail due to a non-empty test directory at the end of a test. Temp dirs created by the `testing` package are _not required to be empty_ at cleanup time, so something has gone awry. After looking at the code, it seems that an some Auth server instances created as fixtures for several tests are never explicitly closed and cleaned up when the test finishes. This leads to a hypothesis that something in the audit writer is writing to the temp dir _while it is in the process of being cleaned up_. This patch add automatic cleanup to the Auth server test fixture, which executes before the temp directory cleanup. The intention is to stop any writes to the temp dir at the end of the test, so that it can be cleaned up without error. Also marks some not-implemented tests _skipped_ rather than _passed_, to increase visibility.
codingllama
approved these changes
Dec 1, 2021
| @@ -0,0 +1,29 @@ | |||
| steps: | |||
| # Run non-root integration tests. The root user is selected by docker image. | |||
| - name: us-docker.pkg.dev/ci-account/teleport/buildbox-root:v0.1.0 | |||
There was a problem hiding this comment.
I suppose this is fine, but should we create a teleport8-v0.1.0 too?
r0mant
approved these changes
Dec 1, 2021
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of this change is implementing a "no secrets" policy for CI. Given that
any test that runs under CI must be able to do so without any injected secrets.
This means that several of the test we currently run under Drone will not be run on GCB, at least as part of the regular CI. The plan is to create a separate task that periodically runs tests that require external credentials (e.g. Kube tests, various backend data stores, etc.) in a more secure way and report failures asynchronously. And while these tests will not run under CI, the should still be built under CI so that required changes are caught during review.
Note: this backport includes various data race fixes added separately in the master branch:
See-Also: #8643
See-Also: #8888
See-Also: #9117
See-Also: #9119