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

deployer: add a bunch of test coverage and fix a few panics #20694

Merged
merged 5 commits into from
Feb 22, 2024
Merged

deployer: add a bunch of test coverage and fix a few panics #20694

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
94c5c75
deployer: add a bunch of test coverage and fix a few panics
rboyer Feb 21, 2024
60a6c1e
mark the integration tests as short
rboyer Feb 21, 2024
f13c915
switch to a build tag to skip integ tests
rboyer Feb 21, 2024
b928ddf
test deployer stuff
rboyer Feb 21, 2024
68474d1
and activate
rboyer Feb 21, 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
23 changes: 22 additions & 1 deletion .github/workflows/go-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,6 +490,26 @@ jobs:
consul-license: ${{secrets.CONSUL_LICENSE}}
datadog-api-key: "${{ !endsWith(github.repository, '-enterprise') && secrets.DATADOG_API_KEY || '' }}"

go-test-testing-deployer:
needs:
- setup
- get-go-version
- dev-build
uses: ./.github/workflows/reusable-unit.yml
with:
directory: testing/deployer
runs-on: ${{ needs.setup.outputs.compute-large }}
repository-name: ${{ github.repository }}
go-tags: "${{ github.event.repository.name == 'consul-enterprise' && 'consulent consuldev' || '' }}"
go-version: ${{ needs.get-go-version.outputs.go-version }}
permissions:
id-token: write # NOTE: this permission is explicitly required for Vault auth.
contents: read
secrets:
elevated-github-token: ${{ secrets.ELEVATED_GITHUB_TOKEN }}
consul-license: ${{secrets.CONSUL_LICENSE}}
datadog-api-key: "${{ !endsWith(github.repository, '-enterprise') && secrets.DATADOG_API_KEY || '' }}"

noop:
runs-on: ubuntu-latest
steps:
Expand Down Expand Up @@ -532,6 +552,7 @@ jobs:
- go-test-sdk-backwards-compatibility
- go-test-sdk
- go-test-32bit
- go-test-testing-deployer
# - go-test-s390x
runs-on: ${{ fromJSON(needs.setup.outputs.compute-small) }}
if: always() && needs.conditional-skip.outputs.skip-ci != 'true'
Expand Down Expand Up @@ -585,4 +606,4 @@ jobs:
"message": ${{ toJSON(env.SLACK_MESSAGE_RAW) }}
}
env:
SLACK_WEBHOOK_URL: ${{ secrets.CONSUL_PROTECTED_BRANCH_TEST_SLACK_WEBHOOK }}
SLACK_WEBHOOK_URL: ${{ secrets.CONSUL_PROTECTED_BRANCH_TEST_SLACK_WEBHOOK }}
6 changes: 4 additions & 2 deletions testing/deployer/sprawl/sprawltest/test_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,21 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: BUSL-1.1

//go:build integration

package sprawltest_test

import (
"strconv"
"testing"

"github.com/stretchr/testify/require"

"github.com/hashicorp/consul/api"
pbauth "github.com/hashicorp/consul/proto-public/pbauth/v2beta1"
pbcatalog "github.com/hashicorp/consul/proto-public/pbcatalog/v2beta1"
pbmesh "github.com/hashicorp/consul/proto-public/pbmesh/v2beta1"
"github.com/hashicorp/consul/proto-public/pbresource"
"github.com/stretchr/testify/require"

"github.com/hashicorp/consul/testing/deployer/sprawl/sprawltest"
"github.com/hashicorp/consul/testing/deployer/topology"
)
Expand Down
104 changes: 81 additions & 23 deletions testing/deployer/topology/compile.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,32 +13,42 @@ import (
"sort"

"github.com/google/go-cmp/cmp"
"golang.org/x/exp/maps"

"github.com/hashicorp/go-hclog"

pbauth "github.com/hashicorp/consul/proto-public/pbauth/v2beta1"
pbcatalog "github.com/hashicorp/consul/proto-public/pbcatalog/v2beta1"
pbmesh "github.com/hashicorp/consul/proto-public/pbmesh/v2beta1"
"github.com/hashicorp/consul/proto-public/pbresource"
"github.com/hashicorp/go-hclog"
"golang.org/x/exp/maps"

"github.com/hashicorp/consul/testing/deployer/util"
)

const DockerPrefix = "cslc" // ConSuLCluster

func Compile(logger hclog.Logger, raw *Config) (*Topology, error) {
return compile(logger, raw, nil)
return compile(logger, raw, nil, "")
Copy link
Collaborator

@NiniOak NiniOak Feb 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't the testingID value be set at a high level? Like wherever compile is called?

Copy link
Member Author

Choose a reason for hiding this comment

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

The tests in compile_test.go call the function this way:

got, err := compile(logger, tc.in, nil, clusterID)

I'm not calling Compile, but rather the unexported compile and using the testingID field which is otherwise inaccessible.

}

func Recompile(logger hclog.Logger, raw *Config, prev *Topology) (*Topology, error) {
Copy link
Collaborator

@NiniOak NiniOak Feb 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why is testingID not added to the parameters required for Recompile function?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would not want to expose the testingID parameter in an exported function yet, as it is just there for the package tests to use.

Possibly if deployer-meta-tests get written for stuff outside of the topology package we might revisit this. It seemed easiest to keep this testing detail internal to this package to start.

if prev == nil {
return nil, errors.New("missing previous topology")
}
return compile(logger, raw, prev)
return compile(logger, raw, prev, "")
}

func compile(logger hclog.Logger, raw *Config, prev *Topology) (*Topology, error) {
func compile(logger hclog.Logger, raw *Config, prev *Topology, testingID string) (*Topology, error) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Adding this so that we can control the id assignment during testing.

if logger == nil {
return nil, errors.New("logger is required")
}
if raw == nil {
return nil, errors.New("config is required")
}

var id string
if prev == nil {
if testingID != "" {
id = testingID
} else if prev == nil {
var err error
id, err = newTopologyID()
if err != nil {
Expand Down Expand Up @@ -90,22 +100,17 @@ func compile(logger hclog.Logger, raw *Config, prev *Topology) (*Topology, error
nextIndex int // use a global index so any shared networks work properly with assignments
)

foundPeerNames := make(map[string]map[string]struct{})
for _, c := range raw.Clusters {
compileCluster := func(c *Cluster) (map[string]struct{}, error) {
Copy link
Member Author

Choose a reason for hiding this comment

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

A follow-on PR can extract these anonymous methods as real methods. I did it this way to make it more obvious where the bugfixes were, and to add error logging context.

if c.Name == "" {
return nil, fmt.Errorf("cluster has no name")
}

foundPeerNames[c.Name] = make(map[string]struct{})
foundPeerNames := make(map[string]struct{})

if !IsValidLabel(c.Name) {
return nil, fmt.Errorf("cluster name is not valid: %s", c.Name)
}

if _, exists := clusters[c.Name]; exists {
return nil, fmt.Errorf("cannot have two clusters with the same name %q; use unique names and override the Datacenter field if that's what you want", c.Name)
}

if c.Datacenter == "" {
c.Datacenter = c.Name
} else {
Expand All @@ -114,7 +119,6 @@ func compile(logger hclog.Logger, raw *Config, prev *Topology) (*Topology, error
}
}

clusters[c.Name] = c
if c.NetworkName == "" {
c.NetworkName = c.Name
}
Expand Down Expand Up @@ -158,6 +162,7 @@ func compile(logger hclog.Logger, raw *Config, prev *Topology) (*Topology, error
m = make(map[string]struct{})
tenancies[partition] = m
}
m["default"] = struct{}{}
Copy link
Member Author

Choose a reason for hiding this comment

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

One of the ways to introduce a tenancy would inject the default, but this one did not. Correcting this omission. The system will automatically create this anyway so this just accurately reflects the tenancies that will exist.

m[namespace] = struct{}{}
}

Expand Down Expand Up @@ -203,8 +208,7 @@ func compile(logger hclog.Logger, raw *Config, prev *Topology) (*Topology, error
addTenancy(res.Id.Tenancy.Partition, res.Id.Tenancy.Namespace)
}

seenNodes := make(map[NodeID]struct{})
for _, n := range c.Nodes {
compileClusterNode := func(n *Node) (map[string]struct{}, error) {
if n.Name == "" {
return nil, fmt.Errorf("cluster %q node has no name", c.Name)
}
Expand Down Expand Up @@ -238,10 +242,9 @@ func compile(logger hclog.Logger, raw *Config, prev *Topology) (*Topology, error
}
addTenancy(n.Partition, "default")

if _, exists := seenNodes[n.ID()]; exists {
return nil, fmt.Errorf("cannot have two nodes in the same cluster %q with the same name %q", c.Name, n.ID())
if n.IsServer() && n.Partition != "default" {
Copy link
Member Author

Choose a reason for hiding this comment

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

Adding this restriction. Practically this would have failed in consul itself, but this makes it fail up higher where it's more actionable.

return nil, fmt.Errorf("server agents must always be in the default partition")
}
seenNodes[n.ID()] = struct{}{}

if len(n.usedPorts) != 0 {
return nil, fmt.Errorf("user cannot specify the usedPorts field")
Expand Down Expand Up @@ -328,7 +331,10 @@ func compile(logger hclog.Logger, raw *Config, prev *Topology) (*Topology, error
return nil, fmt.Errorf("cluster %q node %q uses dataplane, but has more than one service", c.Name, n.Name)
}

seenServices := make(map[ID]struct{})
var (
foundPeerNames = make(map[string]struct{})
seenServices = make(map[ID]struct{})
)
for _, wrk := range n.Workloads {
if n.IsAgent() {
// Default to that of the enclosing node.
Expand Down Expand Up @@ -412,7 +418,7 @@ func compile(logger hclog.Logger, raw *Config, prev *Topology) (*Topology, error
dest.ID.Partition = "" // irrelevant here; we'll set it to the value of the OTHER side for plumbing purposes in tests
}
dest.ID.Namespace = NamespaceOrDefault(dest.ID.Namespace)
foundPeerNames[c.Name][dest.Peer] = struct{}{}
foundPeerNames[dest.Peer] = struct{}{}
}

addTenancy(dest.ID.Partition, dest.ID.Namespace)
Expand All @@ -431,7 +437,7 @@ func compile(logger hclog.Logger, raw *Config, prev *Topology) (*Topology, error
}
if dest.PortName == "" && n.IsV2() {
// Assume this is a v1->v2 conversion and name it.
dest.PortName = "legacy"
dest.PortName = V1DefaultPortName
}
}

Expand Down Expand Up @@ -476,6 +482,15 @@ func compile(logger hclog.Logger, raw *Config, prev *Topology) (*Topology, error
Protocol: cfg.ActualProtocol,
})
}
sort.Slice(svcPorts, func(i, j int) bool {
Copy link
Member Author

Choose a reason for hiding this comment

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

This makes the output deterministic to aid in unit test comparisons.

a, b := svcPorts[i], svcPorts[j]
if a.TargetPort < b.TargetPort {
return true
} else if a.TargetPort > b.TargetPort {
return false
}
return a.Protocol < b.Protocol
})

v2svc := &pbcatalog.Service{
Workloads: &pbcatalog.WorkloadSelector{},
Expand Down Expand Up @@ -520,6 +535,31 @@ func compile(logger hclog.Logger, raw *Config, prev *Topology) (*Topology, error
}
}
}
return foundPeerNames, nil
}

seenNodes := make(map[NodeID]struct{})
for _, n := range c.Nodes {
peerNames, err := compileClusterNode(n)
if err != nil {
return nil, fmt.Errorf("error compiling node %q: %w", n.Name, err)
}

if _, exists := seenNodes[n.ID()]; exists {
return nil, fmt.Errorf("cannot have two nodes in the same cluster %q with the same name %q", c.Name, n.ID())
}
seenNodes[n.ID()] = struct{}{}

maps.Copy(foundPeerNames, peerNames)
}

// Default anything in the toplevel services map.
Copy link
Member Author

Choose a reason for hiding this comment

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

Omission: many of the pathways to implicitly create this v2 services map will default the protocol, but the BYO map does not.

for _, svc := range c.Services {
for _, port := range svc.Ports {
if port.Protocol == pbcatalog.Protocol_PROTOCOL_UNSPECIFIED {
port.Protocol = pbcatalog.Protocol_PROTOCOL_TCP
}
}
}

if err := assignVirtualIPs(c); err != nil {
Expand Down Expand Up @@ -582,6 +622,24 @@ func compile(logger hclog.Logger, raw *Config, prev *Topology) (*Topology, error
return nil, fmt.Errorf("cluster %q references non-default partitions or namespaces but is CE", c.Name)
}
}

return foundPeerNames, nil
}

foundPeerNames := make(map[string]map[string]struct{})
for _, c := range raw.Clusters {
peerNames, err := compileCluster(c)
if err != nil {
return nil, fmt.Errorf("error building cluster %q: %w", c.Name, err)
}

foundPeerNames[c.Name] = peerNames

if _, exists := clusters[c.Name]; exists {
return nil, fmt.Errorf("cannot have two clusters with the same name %q; use unique names and override the Datacenter field if that's what you want", c.Name)
}

clusters[c.Name] = c
}

clusteredPeerings := make(map[string]map[string]*PeerCluster) // local-cluster -> local-peer -> info
Expand Down
Loading
Loading