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

Fix await annotation precedence #3344

Merged
merged 2 commits into from
Dec 3, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@
- [nodejs] Resolves `punycode` deprecation warnings by using native `fetch` instead of `node-fetch`.
(https://github.com/pulumi/pulumi-kubernetes/issues/3301)

### Fixed

Copy link
Member

Choose a reason for hiding this comment

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

Should we also add a note about jsonpath matching on complex types?

- `pulumi.com/waitFor` and other await annotations now correctly take precedence over default await logic.
(https://github.com/pulumi/pulumi-kubernetes/issues/3329)

## 4.18.3 (October 31, 2024)

### Fixed
Expand Down
12 changes: 8 additions & 4 deletions provider/pkg/await/await.go
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ func Creation(c CreateConfig) (*unstructured.Unstructured, error) {
defer cancel()

source := condition.NewDynamicSource(ctx, c.ClientSet, outputs.GetNamespace())
ready, err := metadata.ReadyCondition(ctx, source, c.ClientSet, c.DedupLogger, c.Inputs, outputs)
ready, custom, err := metadata.ReadyCondition(ctx, source, c.ClientSet, c.DedupLogger, c.Inputs, outputs)
if err != nil {
return outputs, err
}
Expand All @@ -281,7 +281,9 @@ func Creation(c CreateConfig) (*unstructured.Unstructured, error) {
if c.awaiters != nil {
a = c.awaiters
}
if spec, ok := a[id]; ok && spec.await != nil {
// Use our built-in await logic only if the user hasn't specified any await
// overrides.
if spec, ok := a[id]; ok && spec.await != nil && !custom {
conf := awaitConfig{
ctx: c.Context,
urn: c.URN,
Expand Down Expand Up @@ -432,7 +434,7 @@ func Update(c UpdateConfig) (*unstructured.Unstructured, error) {
defer cancel()

source := condition.NewDynamicSource(ctx, c.ClientSet, currentOutputs.GetNamespace())
ready, err := metadata.ReadyCondition(ctx, source, c.ClientSet, c.DedupLogger, c.Inputs, currentOutputs)
ready, custom, err := metadata.ReadyCondition(ctx, source, c.ClientSet, c.DedupLogger, c.Inputs, currentOutputs)
if err != nil {
return currentOutputs, err
}
Expand All @@ -441,7 +443,9 @@ func Update(c UpdateConfig) (*unstructured.Unstructured, error) {
if c.awaiters != nil {
a = c.awaiters
}
if spec, ok := a[id]; ok && spec.await != nil {
// Use our built-in await logic only if the user hasn't specified any await
// overrides.
if spec, ok := a[id]; ok && spec.await != nil && !custom {
conf := awaitConfig{
ctx: c.Context,
urn: c.URN,
Expand Down
26 changes: 26 additions & 0 deletions provider/pkg/await/await_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,15 @@ func TestCreation(t *testing.T) {
awaiter: awaitUnexpected,
expect: []expectF{previewed("default", "foo")},
},
{
name: "WaitFor",
args: args{
resType: tokens.Type("kubernetes:core/v1:Pod"),
inputs: withWaitFor(validPodUnstructured),
},
awaiter: awaitUnexpected, // waitFor annotation takes precedence.
expect: []expectF{created("default", "foo")},
},
// FUTURE: test server-side apply (depends on https://github.com/kubernetes/kubernetes/issues/115598)
}

Expand Down Expand Up @@ -625,6 +634,15 @@ func TestUpdate(t *testing.T) {
awaiter: awaitUnexpected,
expect: []expectF{previewed("default", "foo")},
},
{
name: "WaitFor",
args: args{
resType: tokens.Type("kubernetes:core/v1:Pod"),
inputs: withWaitFor(validPodUnstructured),
},
awaiter: awaitUnexpected, // waitFor annotation takes precedence.
expect: []expectF{updated("default", "foo"), logged()},
},
// FUTURE: test server-side apply (depends on https://github.com/kubernetes/kubernetes/issues/115598)
}

Expand Down Expand Up @@ -1054,6 +1072,14 @@ func withSkipAwait(obj *unstructured.Unstructured) *unstructured.Unstructured {
return copy
}

func withWaitFor(obj *unstructured.Unstructured) *unstructured.Unstructured {
copy := obj.DeepCopy()
copy.SetAnnotations(map[string]string{
"pulumi.com/waitFor": "jsonpath={.metadata}", // Succeeds immediately.
})
return copy
}

func withGenerateName(obj *unstructured.Unstructured) *unstructured.Unstructured {
copy := obj.DeepCopy()
copy.SetGenerateName(fmt.Sprintf("%s-", obj.GetName()))
Expand Down
4 changes: 4 additions & 0 deletions provider/pkg/jsonpath/jsonpath.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ func (i *Parsed) Matches(uns *unstructured.Unstructured) (MatchResult, error) {
value := results[0][0]
switch value.Interface().(type) {
case []any, map[string]any:
if i.Value == "" {
// We don't care about complex types if we're matching anything.
return MatchResult{Matched: true}, nil
}
return MatchResult{}, fmt.Errorf("%q has a non-primitive value (%v)", i.Path, value.String())
}
found := fmt.Sprint(value.Interface())
Expand Down
10 changes: 10 additions & 0 deletions provider/pkg/jsonpath/jsonpath_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,16 @@ func TestMatches(t *testing.T) {
}},
want: MatchResult{Matched: true, Found: "<nil>"},
},
{
name: "object exists",
expr: "jsonpath={ .foo }",
uns: &unstructured.Unstructured{Object: map[string]any{
"foo": map[string]any{
"bar": "baz",
},
}},
want: MatchResult{Matched: true},
},
{
name: "key exists with non-primitive value",
expr: "jsonpath={.foo}",
Expand Down
23 changes: 12 additions & 11 deletions provider/pkg/metadata/overrides.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,17 +103,17 @@ func ReadyCondition(
logger *logging.DedupLogger,
inputs *unstructured.Unstructured,
obj *unstructured.Unstructured,
) (condition.Satisfier, error) {
) (condition.Satisfier, bool, error) {
if SkipAwaitLogic(inputs) {
return condition.NewImmediate(logger, obj), nil
return condition.NewImmediate(logger, obj), true, nil
}

val := GetAnnotationValue(obj, AnnotationWaitFor)
if val == "" {
if os.Getenv("PULUMI_K8S_AWAIT_ALL") != "true" {
return condition.NewImmediate(nil, obj), nil
return condition.NewImmediate(nil, obj), false, nil
}
return condition.NewReady(ctx, source, logger, obj), nil
return condition.NewReady(ctx, source, logger, obj), false, nil
}

// Attempt to interpret the annotation as a JSON string array, and if that
Expand All @@ -124,7 +124,7 @@ func ReadyCondition(
values = append(values, val)
}
if len(values) == 0 {
return nil, fmt.Errorf("at least one condition must be specified")
return nil, false, fmt.Errorf("at least one condition must be specified")
}

conditions := make([]condition.Satisfier, 0, len(values))
Expand All @@ -133,28 +133,29 @@ func ReadyCondition(
case strings.HasPrefix(expr, "jsonpath="):
jsp, err := jsonpath.Parse(expr)
if err != nil {
return nil, err
return nil, false, err
}
cond, err := condition.NewJSONPath(ctx, source, logger, obj, jsp)
if err != nil {
return nil, err
return nil, false, err
}
conditions = append(conditions, cond)
case strings.HasPrefix(expr, "condition="):
cond, err := condition.NewCustom(ctx, source, logger, obj, expr)
if err != nil {
return nil, err
return nil, false, err
}
conditions = append(conditions, cond)
default:
return nil, fmt.Errorf(`expected a "jsonpath=" or "condition=" prefix, got %q`, expr)
return nil, false, fmt.Errorf(`expected a "jsonpath=" or "condition=" prefix, got %q`, expr)
}
}

if len(conditions) == 1 {
return conditions[0], nil
return conditions[0], true, nil
}
return condition.NewAll(conditions...)
all, err := condition.NewAll(conditions...)
return all, true, err
}

// DeletedCondition inspects the object's annotations and returns a
Expand Down
20 changes: 14 additions & 6 deletions provider/pkg/metadata/overrides_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ func TestReadyCondition(t *testing.T) {
inputs *unstructured.Unstructured
genericEnabled bool
want any
wantCustom bool
wantErr string
}{
{
Expand All @@ -164,7 +165,8 @@ func TestReadyCondition(t *testing.T) {
},
},
},
want: condition.Immediate{},
want: condition.Immediate{},
wantCustom: true,
},
{
name: "skipAwait=true, generic await enabled",
Expand All @@ -179,6 +181,7 @@ func TestReadyCondition(t *testing.T) {
},
genericEnabled: true,
want: condition.Immediate{},
wantCustom: true,
},
{
name: "skipAwait=true with custom ready condition",
Expand All @@ -190,7 +193,8 @@ func TestReadyCondition(t *testing.T) {
},
},
}},
want: condition.Immediate{},
want: condition.Immediate{},
wantCustom: true,
},
{
name: "skipAwait=false with custom ready condition",
Expand All @@ -202,7 +206,8 @@ func TestReadyCondition(t *testing.T) {
},
},
}},
want: &condition.JSONPath{},
want: &condition.JSONPath{},
wantCustom: true,
},
{
name: "parse JSON array",
Expand All @@ -213,7 +218,8 @@ func TestReadyCondition(t *testing.T) {
},
},
}},
want: &condition.All{},
want: &condition.All{},
wantCustom: true,
},
{
name: "parse empty array",
Expand All @@ -235,7 +241,8 @@ func TestReadyCondition(t *testing.T) {
},
},
}},
want: &condition.JSONPath{},
want: &condition.JSONPath{},
wantCustom: true,
},
{
name: "invalid expression",
Expand All @@ -259,12 +266,13 @@ func TestReadyCondition(t *testing.T) {
if obj == nil {
obj = tt.inputs
}
cond, err := ReadyCondition(context.Background(), nil, nil, nil, tt.inputs, obj)
cond, custom, err := ReadyCondition(context.Background(), nil, nil, nil, tt.inputs, obj)
if tt.wantErr != "" {
assert.ErrorContains(t, err, tt.wantErr)
return
}
assert.IsType(t, tt.want, cond)
assert.Equal(t, tt.wantCustom, custom)
})
}
}
Expand Down
Loading