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

Improve handling build id in view page #651

Merged
merged 8 commits into from
Sep 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 47 additions & 3 deletions src/main/groovy/io/seqera/wave/controller/ViewController.groovy
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,12 @@

package io.seqera.wave.controller

import groovy.json.JsonOutput
import io.micronaut.core.annotation.Nullable
import java.util.regex.Pattern

import groovy.transform.CompileStatic
import groovy.util.logging.Slf4j
import io.micronaut.context.annotation.Value
import io.micronaut.core.annotation.Nullable
import io.micronaut.http.HttpResponse
import io.micronaut.http.annotation.Controller
import io.micronaut.http.annotation.Get
Expand All @@ -46,6 +47,7 @@ import static io.seqera.wave.util.DataTimeUtils.formatTimestamp
*
* @author Paolo Di Tommaso <[email protected]>
*/
@Slf4j
@CompileStatic
@Controller("/view")
@ExecuteOn(TaskExecutors.IO)
Expand All @@ -70,13 +72,55 @@ class ViewController {

@View("build-view")
@Get('/builds/{buildId}')
HttpResponse<Map<String,String>> viewBuild(String buildId) {
HttpResponse viewBuild(String buildId) {
// check redirection for invalid suffix in the form `-nn`
final r1 = shouldRedirect1(buildId)
if( r1 ) {
log.debug "Redirect to build page [1]: $r1"
return HttpResponse.redirect(URI.create(r1))
}
// check redirection when missing the suffix `_nn`
final r2 = shouldRedirect2(buildId)
if( r2 ) {
log.debug "Redirect to build page [2]: $r2"
return HttpResponse.redirect(URI.create(r2))
}
// go ahead with proper handling
final record = buildService.getBuildRecord(buildId)
if( !record )
throw new NotFoundException("Unknown build id '$buildId'")
return HttpResponse.ok(renderBuildView(record))
}

static final private Pattern DASH_SUFFIX = ~/([0-9a-zA-Z\-]+)-(\d+)$/

static final private Pattern MISSING_SUFFIX = ~/([0-9a-zA-Z\-]+)(?<!_\d{2})$/

protected String shouldRedirect1(String buildId) {
// check for build id containing a -nn suffix
final check1 = DASH_SUFFIX.matcher(buildId)
if( check1.matches() ) {
return "/view/builds/${check1.group(1)}_${check1.group(2)}"
}
return null
}

protected String shouldRedirect2(String buildId) {
// check build id missing the _nn suffix
if( !MISSING_SUFFIX.matcher(buildId).matches() )
return null

final rec = buildService.getLatestBuild(buildId)
if( !rec )
return null
if( !rec.buildId.startsWith(buildId) )
return null
if( rec.buildId==buildId )
return null

return "/view/builds/${rec.buildId}"
}

Map<String,String> renderBuildView(WaveBuildRecord result) {
// create template binding
final binding = new HashMap(20)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ import static io.seqera.wave.util.StringUtils.trunc
@CompileStatic
class BuildRequest {

static final String SEP = '_'
static final public String SEP = '_'

/**
* Unique request Id. This is computed as a consistent hash generated from
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,4 +84,13 @@ interface ContainerBuildService {
* @return The {@link WaveBuildRecord} associated with the corresponding Id, or {@code null} if it cannot be found
*/
WaveBuildRecord getBuildRecord(String buildId)

/**
* Retrieve the latest build record available for the specified container id.
*
* @param containerId The ID of the container for which the build record needs to be retrieve
* @return The {@link WaveBuildRecord} associated with the corresponding Id, or {@code null} if it cannot be found
*/
WaveBuildRecord getLatestBuild(String containerId)

}
Original file line number Diff line number Diff line change
Expand Up @@ -422,4 +422,8 @@ class ContainerBuildServiceImpl implements ContainerBuildService, JobHandler<Bui
return buildRecordStore.getBuildRecord(buildId) ?: persistenceService.loadBuild(buildId)
}

@Override
WaveBuildRecord getLatestBuild(String containerId) {
return persistenceService.latestBuild(containerId)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,14 @@ interface PersistenceService {
*/
WaveBuildRecord loadBuild(String targetImage, String digest)

/**
* Retrieve the latest {@link WaveBuildRecord} object for the given container id
*
* @param containerId The container id for which the latest build record should be retrieved
* @return The corresponding {@link WaveBuildRecord} object or {@code null} if no record is found
*/
WaveBuildRecord latestBuild(String containerId)

/**
* Store a {@link WaveContainerRecord} object in the Surreal wave_request table.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,15 @@ class LocalPersistenceService implements PersistenceService {
return buildStore.get(buildId)
}

@Override
WaveBuildRecord latestBuild(String containerId) {
buildStore
.values()
.findAll( it-> it.buildId.startsWith(containerId) )
.sort( it-> it.startTime )
.reverse() [0]
}

@Override
WaveBuildRecord loadBuild(String targetImage, String digest) {
buildStore.values().find( (build) -> build.targetImage==targetImage && build.digest==digest )
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ class SurrealPersistenceService implements PersistenceService {
throw new IllegalStateException("Unable to define SurrealDB table wave_scan_vuln - cause: $ret4")
}

private String getAuthorization() {
protected String getAuthorization() {
"Basic "+"$user:$password".bytes.encodeBase64()
}

Expand Down Expand Up @@ -146,6 +146,21 @@ class SurrealPersistenceService implements PersistenceService {
return result
}

@Override
WaveBuildRecord latestBuild(String containerId) {
final query = """
select *
from wave_build
where buildId ~ '${containerId}${BuildRequest.SEP}'
order by startTime desc limit 1
""".stripIndent()
final json = surrealDb.sqlAsString(getAuthorization(), query)
final type = new TypeReference<ArrayList<SurrealResult<WaveBuildRecord>>>() {}
final data= json ? JacksonHelper.fromJson(json, type) : null
final result = data && data[0].result ? data[0].result[0] : null
return result
}

@Override
void saveContainerRequest(String token, WaveContainerRecord data) {
surrealDb.insertContainerRequestAsync(authorization, token, data).subscribe({ result->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package io.seqera.wave.controller

import spock.lang.Specification
import spock.lang.Unroll

import java.time.Duration
import java.time.Instant
Expand All @@ -32,9 +33,8 @@ import io.micronaut.test.extensions.spock.annotation.MicronautTest
import io.seqera.wave.api.ContainerConfig
import io.seqera.wave.api.SubmitContainerTokenRequest
import io.seqera.wave.core.ContainerPlatform
import io.seqera.wave.core.spec.ContainerSpec
import io.seqera.wave.exception.DockerRegistryException
import io.seqera.wave.service.ContainerRequestData
import io.seqera.wave.service.builder.ContainerBuildService
import io.seqera.wave.service.inspect.ContainerInspectService
import io.seqera.wave.service.logs.BuildLogService
import io.seqera.wave.service.logs.BuildLogServiceImpl
Expand Down Expand Up @@ -74,7 +74,7 @@ class ViewControllerTest extends Specification {
@Inject
private ContainerInspectService inspectService

def 'should render build page' () {
def 'should return build page mapping' () {
given:
def controller = new ViewController(serverUrl: 'http://foo.com', buildLogService: buildLogService)
and:
Expand Down Expand Up @@ -116,7 +116,7 @@ class ViewControllerTest extends Specification {
binding.build_failed == false
}

def 'should render a build page' () {
def 'should render build page' () {
given:
def record1 = new WaveBuildRecord(
buildId: '112233',
Expand Down Expand Up @@ -144,7 +144,7 @@ class ViewControllerTest extends Specification {
!response.body().contains('Conda file')
}

def 'should render a build page with conda file' () {
def 'should render build page with conda file' () {
given:
def record1 = new WaveBuildRecord(
buildId: 'test',
Expand Down Expand Up @@ -327,4 +327,45 @@ class ViewControllerTest extends Specification {
binding.build_url == 'http://foo.com/view/builds/12345'
}

@Unroll
def 'should validate redirection check' () {
given:
def service = Mock(ContainerBuildService)
def controller = new ViewController(buildService: service)

when:
def result = controller.shouldRedirect1(BUILD)
then:
result == EXPECTED

where:
BUILD | EXPECTED
'12345_1' | null
'12345-1' | '/view/builds/12345_1'
'foo-887766-1' | '/view/builds/foo-887766_1'

}


def 'should validate redirect 2' () {
given:
def service = Mock(ContainerBuildService)
def controller = new ViewController(buildService: service)

when:
def result = controller.shouldRedirect2(BUILD)
then:
result == EXPECTED
TIMES * service.getLatestBuild(BUILD) >> LATEST

where:
BUILD | TIMES | LATEST | EXPECTED
'12345_1' | 0 | null | null
'12345' | 1 | Mock(WaveBuildRecord) { buildId >> '12345_99' } | '/view/builds/12345_99'
'12345' | 1 | Mock(WaveBuildRecord) { buildId >> 'xyz_99' } | null
'foo-887766' | 1 | Mock(WaveBuildRecord) { buildId >> 'foo-887766_99' } | '/view/builds/foo-887766_99'
'foo-887766' | 1 | Mock(WaveBuildRecord) { buildId >> 'foo-887766' } | null

}

}
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,31 @@ class SurrealPersistenceServiceTest extends Specification implements SurrealDBTe
loaded == record
}

def 'should find latest build' () {
given:
def surreal = applicationContext.getBean(SurrealClient)
def persistence = applicationContext.getBean(SurrealPersistenceService)
def auth = persistence.getAuthorization()
def request1 = new BuildRequest( containerId: 'abc', workspace: Path.of('.'), startTime: Instant.now().minusSeconds(30), identity: PlatformId.NULL ).withBuildId('1')
def request2 = new BuildRequest( containerId: 'abc', workspace: Path.of('.'), startTime: Instant.now().minusSeconds(20), identity: PlatformId.NULL ).withBuildId('2')
def request3 = new BuildRequest( containerId: 'abc', workspace: Path.of('.'), startTime: Instant.now().minusSeconds(10), identity: PlatformId.NULL ).withBuildId('3')

def result1 = new BuildResult(request1.buildId, -1, "ok", request1.startTime, Duration.ofSeconds(2), null)
surreal.insertBuild(auth, WaveBuildRecord.fromEvent(new BuildEvent(request1, result1)))
and:
def result2 = new BuildResult(request2.buildId, -1, "ok", request2.startTime, Duration.ofSeconds(2), null)
surreal.insertBuild(auth, WaveBuildRecord.fromEvent(new BuildEvent(request2, result2)))
and:
def result3 = new BuildResult(request3.buildId, -1, "ok", request3.startTime, Duration.ofSeconds(2), null)
surreal.insertBuild(auth, WaveBuildRecord.fromEvent(new BuildEvent(request3, result3)))

when:
def loaded = persistence.latestBuild('abc')

then:
loaded.buildId == 'abc_3'
}

def 'should save and update a build' () {
given:
def persistence = applicationContext.getBean(SurrealPersistenceService)
Expand Down