Skip to content

Commit

Permalink
fix: improve freight availability checks (#2724)
Browse files Browse the repository at this point in the history 
Signed-off-by: Kent Rancourt <[email protected]>
  • Loading branch information
@krancour
krancour authored Oct 11, 2024
1 parent 2b36c75 commit 2bb71f2
Show file tree
Hide file tree
Showing 10 changed files with 451 additions and 130 deletions.
42 changes: 16 additions & 26 deletions api/v1alpha1/freight_helpers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,34 +86,24 @@ func GetFreightByAlias(
}

// IsFreightAvailable answers whether the specified Freight is available to the
// specified Stage having the specified upstream stages. Freight is available
// if:
//
// 1. No upstreamStages are specified
// OR
// 2. The Freight has been verified in ANY of the specified upstream stages
// OR
// 3. The Freight is approved for the specified stage
//
// Note: The rationale for returning true when no upstream stages are specified
// is that some Stages have no upstream Stages (e.g. a Stage that subscribes to
// a Warehouse), so ANY Freight is available to such a Stage.
func IsFreightAvailable(
freight *Freight,
stage string,
upstreamStages []string,
) bool {
if len(upstreamStages) == 0 {
return true
// specified Stage.
func IsFreightAvailable(stage *Stage, freight *Freight) bool {
if stage == nil || freight == nil || stage.Namespace != freight.Namespace {
return false
}
for _, stage := range upstreamStages {
if _, ok := freight.Status.VerifiedIn[stage]; ok {
return true
}
if _, approved := freight.Status.ApprovedFor[stage.Name]; approved {
return true
}
if stage != "" {
if _, ok := freight.Status.ApprovedFor[stage]; ok {
return true
for _, req := range stage.Spec.RequestedFreight {
if freight.Origin.Equals(&req.Origin) {
if req.Sources.Direct {
return true
}
for _, source := range req.Sources.Stages {
if _, verified := freight.Status.VerifiedIn[source]; verified {
return true
}
}
}
}
return false
Expand Down
173 changes: 141 additions & 32 deletions api/v1alpha1/freight_helpers_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,54 +116,163 @@ func TestGetFreightByAlias(t *testing.T) {
}

func TestIsFreightAvailable(t *testing.T) {
testFreight := &Freight{
Status: FreightStatus{
VerifiedIn: map[string]VerifiedStage{
"fake-stage-1": {},
const testNamespace = "fake-namespace"
const testWarehouse = "fake-warehouse"
const testStage = "fake-stage"

testCases := []struct {
name string
stage *Stage
Freight *Freight
expected bool
}{
{
name: "stage is nil",
expected: false,
},
{
name: "freight is nil",
expected: false,
},
{
name: "stage and freight are in different namespaces",
stage: &Stage{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
},
},
ApprovedFor: map[string]ApprovedStage{
"fake-stage-2": {},
Freight: &Freight{
ObjectMeta: metav1.ObjectMeta{
Namespace: "wrong-namespace",
},
},
expected: false,
},
}
testCases := []struct {
name string
stage string
upstreamStages []string
available bool
}{
{
name: "no upstream Stages specified",
available: true,
name: "freight is approved for stage",
stage: &Stage{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
Name: testStage,
},
},
Freight: &Freight{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
Name: testStage,
},
Status: FreightStatus{
ApprovedFor: map[string]ApprovedStage{
testStage: {},
},
},
},
expected: true,
},
{
name: "verified in an upstream Stage",
upstreamStages: []string{"fake-stage-1"},
available: true,
name: "stage accepts freight direct from origin",
stage: &Stage{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
},
Spec: StageSpec{
RequestedFreight: []FreightRequest{{
Origin: FreightOrigin{
Kind: FreightOriginKindWarehouse,
Name: testWarehouse,
},
Sources: FreightSources{
Direct: true,
},
}},
},
},
Freight: &Freight{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
},
Origin: FreightOrigin{
Kind: FreightOriginKindWarehouse,
Name: testWarehouse,
},
},
expected: true,
},
{
name: "approved for Stage",
stage: "fake-stage-2",
upstreamStages: []string{"fake-stage-3"},
available: true,
name: "freight is verified in an upstream stage",
stage: &Stage{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
},
Spec: StageSpec{
RequestedFreight: []FreightRequest{{
Origin: FreightOrigin{
Kind: FreightOriginKindWarehouse,
Name: testWarehouse,
},
Sources: FreightSources{
Stages: []string{"upstream-stage"},
},
}},
},
},
Freight: &Freight{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
},
Origin: FreightOrigin{
Kind: FreightOriginKindWarehouse,
Name: testWarehouse,
},
Status: FreightStatus{
VerifiedIn: map[string]VerifiedStage{
"upstream-stage": {},
},
},
},
expected: true,
},
{
name: "unavailable",
stage: "fake-stage-3",
upstreamStages: []string{"upstream-stage-2"},
available: false,
name: "freight from origin not requested",
stage: &Stage{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
},
Spec: StageSpec{
RequestedFreight: []FreightRequest{{
Origin: FreightOrigin{
Kind: FreightOriginKindWarehouse,
Name: testWarehouse,
},
Sources: FreightSources{
Stages: []string{"upstream-stage"},
},
}},
},
},
Freight: &Freight{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
},
Origin: FreightOrigin{
Kind: FreightOriginKindWarehouse,
Name: "wrong-warehouse",
},
Status: FreightStatus{
VerifiedIn: map[string]VerifiedStage{
"upstream-stage": {},
},
},
},
expected: false,
},
}
for _, testCase := range testCases {
t.Run(testCase.name, func(t *testing.T) {
require.Equal(
t,
testCase.available,
IsFreightAvailable(
testFreight,
testCase.stage,
testCase.upstreamStages,
),
testCase.expected,
IsFreightAvailable(testCase.stage, testCase.Freight),
)
})
}
Expand Down
29 changes: 20 additions & 9 deletions internal/api/promote_downstream_v1alpha1.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,22 +92,26 @@ func (s *server) PromoteDownstream(
}
return nil, connect.NewError(connect.CodeNotFound, err)
}
if !s.isFreightAvailableFn(
freight,
"", // approved for not considered
[]string{stageName}, // verified in
) {

var directAllowed bool
for _, req := range stage.Spec.RequestedFreight {
if req.Origin.Equals(&freight.Origin) && req.Sources.Direct {
directAllowed = true
break
}
}
if _, verified := freight.Status.VerifiedIn[stage.Name]; !verified && !directAllowed {
return nil, connect.NewError(
connect.CodeInvalidArgument,
fmt.Errorf(
"Freight %q is not available to Stage %q",
"Freight %q is not available to Stages down stream from %q",
freightName,
stageName,
),
)
}

downstreams, err := s.findDownstreamStagesFn(ctx, stage)
downstreams, err := s.findDownstreamStagesFn(ctx, stage, freight.Origin)
if err != nil {
return nil, fmt.Errorf("find downstream stages: %w", err)
}
Expand Down Expand Up @@ -160,16 +164,23 @@ func (s *server) PromoteDownstream(
}

// findDownstreamStages returns a list of Stages that are immediately downstream
// from the given Stage.
// from the given Stage and request Freight from the given origin.
// TODO: this could be powered by an index.
func (s *server) findDownstreamStages(ctx context.Context, stage *kargoapi.Stage) ([]kargoapi.Stage, error) {
func (s *server) findDownstreamStages(
ctx context.Context,
stage *kargoapi.Stage,
origin kargoapi.FreightOrigin,
) ([]kargoapi.Stage, error) {
var allStages kargoapi.StageList
if err := s.client.List(ctx, &allStages, client.InNamespace(stage.Namespace)); err != nil {
return nil, connect.NewError(connect.CodeInternal, err)
}
var downstreams []kargoapi.Stage
for _, s := range allStages.Items {
for _, req := range s.Spec.RequestedFreight {
if !req.Origin.Equals(&origin) {
continue
}
for _, upstream := range req.Sources.Stages {
if upstream == stage.Name {
downstreams = append(downstreams, s)
Expand Down
Loading

0 comments on commit 2bb71f2

Please sign in to comment.