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.

Sign up for GitHub

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

Jump to bottom

Revert custom contextual loggers changes #2337

Merged
merged 1 commit into from
Jan 15, 2020
Merged

Revert custom contextual loggers changes #2337

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
96 changes: 43 additions & 53 deletions agent/acs/handler/payload_handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,9 @@ package handler

import (
"context"
"encoding/json"
"errors"
"fmt"
"runtime"
"reflect"
"sync"
"testing"

Expand All @@ -34,11 +33,11 @@ import (
"github.com/aws/amazon-ecs-agent/agent/eventhandler"
"github.com/aws/amazon-ecs-agent/agent/statemanager"
mock_statemanager "github.com/aws/amazon-ecs-agent/agent/statemanager/mocks"
"github.com/aws/amazon-ecs-agent/agent/taskresource"
mock_wsclient "github.com/aws/amazon-ecs-agent/agent/wsclient/mock"
"github.com/aws/aws-sdk-go/aws"
"github.com/golang/mock/gomock"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

const (
Expand Down Expand Up @@ -148,7 +147,13 @@ func TestHandlePayloadMessageStateSaveError(t *testing.T) {
})
assert.Error(t, err, "Expected error while adding a task from statemanager")

validateTask(t, addedTask, "t1")
// We expect task to be added to the engine even though it hasn't been saved
expectedTask := &apitask.Task{
Arn: "t1",
ResourcesMapUnsafe: make(map[string][]taskresource.TaskResource),
}

assert.Equal(t, addedTask, expectedTask, "added task is not expected")
}

// TestHandlePayloadMessageAckedWhenTaskAdded tests if the handler generates an ack
Expand Down Expand Up @@ -189,7 +194,12 @@ func TestHandlePayloadMessageAckedWhenTaskAdded(t *testing.T) {
// Verify the message id acked
assert.Equal(t, aws.StringValue(ackRequested.MessageId), payloadMessageId, "received message is not expected")

validateTask(t, addedTask, "t1")
// Verify if task added == expected task
expectedTask := &apitask.Task{
Arn: "t1",
ResourcesMapUnsafe: make(map[string][]taskresource.TaskResource),
}
assert.Equal(t, addedTask, expectedTask, "received task is not expected")
}

// TestHandlePayloadMessageCredentialsAckedWhenTaskAdded tests if the handler generates
Expand Down Expand Up @@ -280,7 +290,8 @@ func TestHandlePayloadMessageCredentialsAckedWhenTaskAdded(t *testing.T) {
SessionToken: credentialsSessionToken,
CredentialsID: credentialsId,
}
validateTaskAndCredentials(t, taskCredentialsAckRequested, expectedCredentialsAck, addedTask, taskArn, expectedCredentials, "t1")
err = validateTaskAndCredentials(taskCredentialsAckRequested, expectedCredentialsAck, addedTask, taskArn, expectedCredentials)
assert.NoError(t, err, "error validating added task or credentials ack for the same")
}

// TestAddPayloadTaskAddsNonStoppedTasksAfterStoppedTasks tests if tasks with desired status
Expand Down Expand Up @@ -360,7 +371,12 @@ func TestPayloadBufferHandler(t *testing.T) {
// Verify if payloadMessageId read from the ack buffer is correct
assert.Equal(t, aws.StringValue(ackRequested.MessageId), payloadMessageId, "received task is not expected")

validateTask(t, addedTask, "t1")
// Verify if the task added to the engine is correct
expectedTask := &apitask.Task{
Arn: taskArn,
ResourcesMapUnsafe: make(map[string][]taskresource.TaskResource),
}
assert.Equal(t, addedTask, expectedTask, "received task is not expected")
}

// TestPayloadBufferHandlerWithCredentials tests if the async payloadBufferHandler routine
Expand Down Expand Up @@ -479,7 +495,8 @@ func TestPayloadBufferHandlerWithCredentials(t *testing.T) {
SessionToken: firstTaskCredentialsSessionToken,
CredentialsID: firstTaskCredentialsId,
}
validateTaskAndCredentials(t, firstTaskCredentialsAckRequested, expectedCredentialsAckForFirstTask, firstAddedTask, firstTaskArn, expectedCredentialsForFirstTask, "t1")
err := validateTaskAndCredentials(firstTaskCredentialsAckRequested, expectedCredentialsAckForFirstTask, firstAddedTask, firstTaskArn, expectedCredentialsForFirstTask)
assert.NoError(t, err, "error validating added task or credentials ack for the same")

// Verify the correctness of the second task added to the engine and the
// credentials ack generated for it
Expand All @@ -496,7 +513,8 @@ func TestPayloadBufferHandlerWithCredentials(t *testing.T) {
SessionToken: secondTaskCredentialsSessionToken,
CredentialsID: secondTaskCredentialsId,
}
validateTaskAndCredentials(t, secondTaskCredentialsAckRequested, expectedCredentialsAckForSecondTask, secondAddedTask, secondTaskArn, expectedCredentialsForSecondTask, "t2")
err = validateTaskAndCredentials(secondTaskCredentialsAckRequested, expectedCredentialsAckForSecondTask, secondAddedTask, secondTaskArn, expectedCredentialsForSecondTask)
assert.NoError(t, err, "error validating added task or credentials ack for the same")
}

// TestAddPayloadTaskAddsExecutionRoles tests the payload handler will add
Expand Down Expand Up @@ -578,18 +596,24 @@ func TestAddPayloadTaskAddsExecutionRoles(t *testing.T) {
// validateTaskAndCredentials compares a task and a credentials ack object
// against expected values. It returns an error if either of the the
// comparisons fail
func validateTaskAndCredentials(
t *testing.T,
taskCredentialsAck *ecsacs.IAMRoleCredentialsAckRequest,
expectedCredentialsAckForTask *ecsacs.IAMRoleCredentialsAckRequest,
func validateTaskAndCredentials(taskCredentialsAck, expectedCredentialsAckForTask *ecsacs.IAMRoleCredentialsAckRequest,
addedTask *apitask.Task,
expectedTaskArn string,
expectedTaskCredentials credentials.IAMRoleCredentials,
taskName string,
) {
require.Equal(t, expectedCredentialsAckForTask, taskCredentialsAck)
require.Equal(t, expectedTaskCredentials.CredentialsID, addedTask.GetCredentialsID())
validateTask(t, addedTask, taskName)
expectedTaskCredentials credentials.IAMRoleCredentials) error {
if !reflect.DeepEqual(taskCredentialsAck, expectedCredentialsAckForTask) {
return fmt.Errorf("Mismatch between expected and received credentials ack requests, expected: %s, got: %s", expectedCredentialsAckForTask.String(), taskCredentialsAck.String())
}

expectedTask := &apitask.Task{
Arn: expectedTaskArn,
ResourcesMapUnsafe: make(map[string][]taskresource.TaskResource),
}
expectedTask.SetCredentialsID(expectedTaskCredentials.CredentialsID)

if !reflect.DeepEqual(addedTask, expectedTask) {
return fmt.Errorf("Mismatch between expected and added tasks, expected: %v, added: %v", expectedTask, addedTask)
}
return nil
}

func TestPayloadHandlerAddedENIToTask(t *testing.T) {
Expand Down Expand Up @@ -925,37 +949,3 @@ func TestPayloadHandlerAddedFirelensData(t *testing.T) {
assert.NotNil(t, actual.Options)
assert.Equal(t, aws.StringValue(expected.Options["enable-ecs-log-metadata"]), actual.Options["enable-ecs-log-metadata"])
}

func validateTask(t *testing.T, addedTask *apitask.Task, expectedTaskName string) {
// We expect task to be added to the engine even though it hasn't been saved
addedTaskJSON, err := json.Marshal(addedTask)
require.NoError(t, err)
platformFields := "{}"
if runtime.GOOS == "windows" {
platformFields = `{"cpuUnbounded": false, "memoryUnbounded": false}`
}
expectedTaskJSON := fmt.Sprintf(`
{
"Arn": "%s",
"Family": "",
"Version": "",
"Containers": null,
"associations": null,
"resources": {},
"volumes": null,
"DesiredStatus": "NONE",
"KnownStatus": "NONE",
"KnownTime": "0001-01-01T00:00:00Z",
"PullStartedAt": "0001-01-01T00:00:00Z",
"PullStoppedAt": "0001-01-01T00:00:00Z",
"ExecutionStoppedAt": "0001-01-01T00:00:00Z",
"SentStatus": "NONE",
"StartSequenceNumber": 0,
"StopSequenceNumber": 0,
"executionCredentialsID": "",
"ENI": null,
"AppMesh": null,
"PlatformFields": %s
}`, expectedTaskName, platformFields)
require.JSONEq(t, expectedTaskJSON, string(addedTaskJSON))
}
4 changes: 2 additions & 2 deletions agent/api/container/container.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1001,7 +1001,7 @@ func (c *Container) GetLogDriver() string {
hostConfig := &dockercontainer.HostConfig{}
err := json.Unmarshal([]byte(*c.DockerConfig.HostConfig), hostConfig)
if err != nil {
seelog.Warnf("Encountered error when trying to get log driver for container %s: %v", c.String(), err)
seelog.Warnf("Encountered error when trying to get log driver for container %s: %v", err)
return ""
}

Expand All @@ -1021,7 +1021,7 @@ func (c *Container) GetNetworkModeFromHostConfig() string {
// TODO return error to differentiate between error and default mode .
err := json.Unmarshal([]byte(*c.DockerConfig.HostConfig), hostConfig)
if err != nil {
seelog.Warnf("Encountered error when trying to get network mode for container %s: %v", c.String(), err)
seelog.Warnf("Encountered error when trying to get network mode for container %s: %v", err)
return ""
}

Expand Down
14 changes: 0 additions & 14 deletions agent/api/task/json.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,3 @@ func (t *Task) MarshalJSON() ([]byte, error) {

return json.Marshal((*jTask)(t))
}

// UnmarshalJSON wraps Go's unmarshalling logic to guarantee that the logger gets created
func (t *Task) UnmarshalJSON(data []byte) error {
err := json.Unmarshal(data, (*jTask)(t))
if err != nil {
return err
}
t.log.SetContext(map[string]string{
"taskARN": t.Arn,
"taskFamily": t.Family,
"taskVersion": t.Version,
})
return nil
}
Loading