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

Feature/4890 detect fail early upgrade #5864

Merged
merged 39 commits into from
Nov 8, 2024
Merged

Feature/4890 detect fail early upgrade #5864

Changes from 1 commit
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
57e54a0
feature(4890): added shouldUpgrade function in the upgrade cli file
kaanyalti Oct 24, 2024
94ad0c3
feature(4890): added shouldUpgrade check into the upgrade command
kaanyalti Oct 24, 2024
3a4b97c
feature(4890): ran gofmt
kaanyalti Oct 24, 2024
accbd22
feature(4890): added a "force" flag, marked it as hidden
kaanyalti Oct 24, 2024
0339dff
feature(4890): removed dpkg, rpm and container logic
kaanyalti Oct 25, 2024
36b3a23
feature(4890): ran gofmt
kaanyalti Oct 25, 2024
e9f4fb5
feature(4890): updated the function signature of the upgrade command,…
kaanyalti Oct 26, 2024
b0b9b82
feature(4890): update comments
kaanyalti Oct 26, 2024
05195ae
feature(4890): added changelog fragment
kaanyalti Oct 26, 2024
21b09af
feature(4890): added fatal log in case there is an error while markin…
kaanyalti Oct 26, 2024
7853878
feature(4890): added error checks in tests
kaanyalti Oct 26, 2024
89f3e0d
feature(4890): updated the summary in the changelog fragment
kaanyalti Oct 28, 2024
0e9b791
feature(4890): removed the shorthand flag for the force flag
kaanyalti Oct 28, 2024
621b465
feature(4890): updated synchronization in the tests
kaanyalti Oct 28, 2024
7acd42e
Update internal/pkg/agent/cmd/upgrade_test.go
kaanyalti Oct 28, 2024
7c66dd1
feature(4890): using streams err output instead of defaulting to stderr
kaanyalti Oct 28, 2024
fde8ef3
feature(4890): use EXPECT instead of On
kaanyalti Oct 28, 2024
dabd25e
feature(4890): moved unconfirmed upgrade error to a package var
kaanyalti Oct 28, 2024
54d4da3
feature(4890): removed confirmation from upgrade check for when force…
kaanyalti Oct 29, 2024
073222c
Update internal/pkg/agent/cmd/upgrade.go
kaanyalti Oct 31, 2024
f2f5691
Update internal/pkg/agent/cmd/upgrade.go
kaanyalti Oct 31, 2024
5c6d62c
feature(4890): fix errors
kaanyalti Oct 31, 2024
1c1f99a
Update internal/pkg/agent/cmd/upgrade.go
kaanyalti Nov 1, 2024
3df1347
feature(4890): update test
kaanyalti Nov 5, 2024
6d26eb2
fearure(4890): replace ageninfo with state call
kaanyalti Nov 6, 2024
6bf8867
feature(4890): updated proto, client and server implementation
kaanyalti Nov 6, 2024
7d99902
feature(4890): fix struct tag
kaanyalti Nov 6, 2024
fe3e91e
feature(4890): added skip-verify checks
kaanyalti Nov 6, 2024
c9320c8
feature(4890): ran addLicenseHeaders
kaanyalti Nov 6, 2024
4601ea3
feature(4890): ran mage clean
kaanyalti Nov 6, 2024
d5d84bc
feature(4890): fix typo
kaanyalti Nov 6, 2024
186fcb1
feature(4890): added timeout to connection
kaanyalti Nov 6, 2024
eb73c7f
feature(4890): changed condition check order
kaanyalti Nov 6, 2024
2eb6b0d
feature(4890): fix unit tests
kaanyalti Nov 7, 2024
8ade18e
feature(4890): refactored tests, using mock client
kaanyalti Nov 7, 2024
6d21a40
Update internal/pkg/agent/cmd/upgrade.go
kaanyalti Nov 7, 2024
a4b27f7
feature(4890): use lower case "f" in error messages to be more consis…
kaanyalti Nov 7, 2024
3de964e
feature(4890): remove duplicate line
kaanyalti Nov 7, 2024
52f8884
feature(4890): ran mage controlProto with correct protoc version
kaanyalti Nov 7, 2024
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
Prev Previous commit
Next Next commit
feature(4890): updated synchronization in the tests
  • Loading branch information
@kaanyalti
kaanyalti committed Nov 7, 2024
commit 621b4657493613a4f7a47af7d5e769470be2e593
79 changes: 59 additions & 20 deletions internal/pkg/agent/cmd/upgrade_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"context"
"log"
"net"
"sync"
"sync/atomic"
"testing"
"time"
Expand Down Expand Up @@ -83,13 +84,13 @@ func TestUpgradeCmd(t *testing.T) {
<-clientCh
})
t.Run("fail if fleet managed and unprivileged", func(t *testing.T) {
var wg sync.WaitGroup
// Set up mock TCP server for gRPC connection
tcpServer, err := net.Listen("tcp", "127.0.0.1:")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only investigate this if you want to, but I think these tests can be simplified because you don't actually need a server to test gRPC server implementations. You can just invoke the mock.Upgrade() RPC implementation directly without needing a network transport to connect you to it.

You would probably have to make a test client type that wraps the server type, so that calling client.Upgrade() just immediately calls mock.Upgrade().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes much more sense. I just looked at the already implemented test case and followed the same approach for the new ones that I added. Comparing the test cases I added to the one that was already implemented, I see that the new ones don't necessarily need an actual server running as we are not testing for connection interruptions. Will look into this one; however, it is not a priority for me. I may open a new PR to refactor these test cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updating this now actually. Instead of mock server, I am using mock client

require.NoError(t, err)
defer tcpServer.Close()

s := grpc.NewServer()
defer s.Stop()

// Define mock server and agent information
upgradeCh := make(chan struct{})
Expand All @@ -99,7 +100,9 @@ func TestUpgradeCmd(t *testing.T) {
mockAgentInfo.On("Unprivileged").Return(true) // Simulate unprivileged mode
cproto.RegisterElasticAgentControlServer(s, mock)

wg.Add(1)
go func() {
defer wg.Done()
err := s.Serve(tcpServer)
assert.NoError(t, err)
}()
Expand All @@ -120,10 +123,11 @@ func TestUpgradeCmd(t *testing.T) {
nil,
}

clientCh := make(chan struct{})

term := make(chan int)
wg.Add(1)
// Execute upgrade command and validate shouldUpgrade error
go func() {
defer wg.Done()
err = upgradeCmdWithClient(commandInput)

// Expect an error due to unprivileged fleet-managed mode
Expand All @@ -133,21 +137,27 @@ func TestUpgradeCmd(t *testing.T) {
// Verify counter has not incremented since upgrade should not proceed
counter := atomic.LoadInt32(&mock.upgrades)
assert.Equal(t, int32(0), counter, "server should not have handled any upgrades")
close(term)
}()

close(clientCh)
wg.Add(1)
go func() {
defer wg.Done()
<-term
s.Stop()
}()

<-clientCh // Ensure goroutine completes before ending test
wg.Wait()
})

t.Run("fail if fleet managed privileged but no force flag", func(t *testing.T) {
var wg sync.WaitGroup
// Set up mock TCP server for gRPC connection
tcpServer, err := net.Listen("tcp", "127.0.0.1:")
require.NoError(t, err)
defer tcpServer.Close()

s := grpc.NewServer()
defer s.Stop()

// Define mock server and agent information
mock := &mockServer{}
Expand All @@ -156,7 +166,9 @@ func TestUpgradeCmd(t *testing.T) {
mockAgentInfo.On("Unprivileged").Return(false) // Simulate privileged mode
cproto.RegisterElasticAgentControlServer(s, mock)

wg.Add(1)
go func() {
defer wg.Done()
err := s.Serve(tcpServer)
assert.NoError(t, err)
}()
Expand All @@ -177,10 +189,11 @@ func TestUpgradeCmd(t *testing.T) {
nil,
}

clientCh := make(chan struct{})

term := make(chan int)
wg.Add(1)
// Execute upgrade command and validate shouldUpgrade error
go func() {
defer wg.Done()
err = upgradeCmdWithClient(commandInput)

// Expect an error due to unprivileged fleet-managed mode
Expand All @@ -190,20 +203,26 @@ func TestUpgradeCmd(t *testing.T) {
// Verify counter has not incremented since upgrade should not proceed
counter := atomic.LoadInt32(&mock.upgrades)
assert.Equal(t, int32(0), counter, "server should not have handled any upgrades")
close(term)
}()

close(clientCh)
wg.Add(1)
go func() {
defer wg.Done()
<-term
s.Stop()
}()

<-clientCh // Ensure goroutine completes before ending test
wg.Wait()
})
t.Run("abort upgrade if fleet managed, privileged, --force is set, and user does not confirm", func(t *testing.T) {
var wg sync.WaitGroup
// Set up mock TCP server for gRPC connection
tcpServer, err := net.Listen("tcp", "127.0.0.1:")
require.NoError(t, err)
defer tcpServer.Close()

s := grpc.NewServer()
defer s.Stop()

// Define mock server and agent information
mock := &mockServer{}
Expand All @@ -212,7 +231,9 @@ func TestUpgradeCmd(t *testing.T) {
mockAgentInfo.On("Unprivileged").Return(false) // Simulate privileged mode
cproto.RegisterElasticAgentControlServer(s, mock)

wg.Add(1)
go func() {
defer wg.Done()
err := s.Serve(tcpServer)
assert.NoError(t, err)
}()
Expand All @@ -239,10 +260,11 @@ func TestUpgradeCmd(t *testing.T) {
},
}

clientCh := make(chan struct{})

term := make(chan int)
wg.Add(1)
// Execute upgrade command and validate shouldUpgrade error
go func() {
defer wg.Done()
err = upgradeCmdWithClient(commandInput)

// Expect an error because user does not confirm the upgrade
Expand All @@ -253,19 +275,26 @@ func TestUpgradeCmd(t *testing.T) {
counter := atomic.LoadInt32(&mock.upgrades)
assert.Equal(t, int32(0), counter, "server should not have handled any upgrades")

close(clientCh)
close(term)
}()

<-clientCh // Ensure goroutine completes before ending test
wg.Add(1)
go func() {
defer wg.Done()
<-term
s.Stop()
}()

wg.Wait()
})
t.Run("proceed with upgrade if fleet managed, privileged, --force is set, and user confirms upgrade", func(t *testing.T) {
var wg sync.WaitGroup
// Set up mock TCP server for gRPC connection
tcpServer, err := net.Listen("tcp", "127.0.0.1:")
require.NoError(t, err)
defer tcpServer.Close()

s := grpc.NewServer()
defer s.Stop()

// Define mock server and agent information
upgradeCh := make(chan struct{})
Expand All @@ -275,7 +304,9 @@ func TestUpgradeCmd(t *testing.T) {
mockAgentInfo.On("Unprivileged").Return(false) // Simulate privileged mode
cproto.RegisterElasticAgentControlServer(s, mock)

wg.Add(1)
go func() {
defer wg.Done()
err := s.Serve(tcpServer)
assert.NoError(t, err)
}()
Expand All @@ -302,10 +333,11 @@ func TestUpgradeCmd(t *testing.T) {
},
}

clientCh := make(chan struct{})

term := make(chan int)
wg.Add(1)
// Execute upgrade command and validate that there are no errors
go func() {
defer wg.Done()
err = upgradeCmdWithClient(commandInput)

assert.NoError(t, err)
Expand All @@ -314,12 +346,19 @@ func TestUpgradeCmd(t *testing.T) {
counter := atomic.LoadInt32(&mock.upgrades)
assert.Equal(t, int32(1), counter, "server should handle exactly one upgrade")

close(clientCh)
close(term)
}()

close(upgradeCh)

<-clientCh // Ensure goroutine completes before ending test
wg.Add(1)
go func() {
defer wg.Done()
<-term
s.Stop()
}()

wg.Wait()
})
}

Expand Down