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

yaml/v2: apply the namespace uniformly to the resource name #2870

Closed
Tracked by #1971
EronWright opened this issue Mar 11, 2024 · 3 comments · Fixed by #2897
Closed
Tracked by #1971

yaml/v2: apply the namespace uniformly to the resource name #2870

EronWright opened this issue Mar 11, 2024 · 3 comments · Fixed by #2897
Assignees
Labels
area/yaml kind/bug Some behavior is incorrect or out of spec resolution/fixed This issue was fixed resolution/wont-fix This issue won't be fixed

Comments

@EronWright
Copy link
Contributor

EronWright commented Mar 11, 2024

There's a minor wart in how the yaml.v2 package chooses to name the child resources, in that it may or may not incorporate the namespace. It would be more consistent if the namespace were always included (for namespaced resources only, of course). The benefit is improved name stability and regularity.

The underlying reason is that the provider includes the namespace only if it is literally specified, not defaulted. It also doesn't know whether a given kind is namespaced (but could use the dynamic client to discover it).

For example, given this manifest we'd get the following stack:

kind: ConfigMap
metadata:
  name: cm1
---
kind: ConfigMap
metadata:
  name: cm2
  namespace: foo
Current stack resources (6):
    TYPE                                NAME
    pulumi:pulumi:Stack                 issue-1971-nodejs-dev
    ├─ kubernetes:yaml/v2:ConfigGroup   cg
    │  ├─ kubernetes:core/v1:ConfigMap  cg-cm1
    │  └─ kubernetes:core/v1:ConfigMap  cg-foo/cm2

I would advocate for the resultant stack to be:

Current stack resources (6):
    TYPE                                NAME
    pulumi:pulumi:Stack                 issue-1971-nodejs-dev
    ├─ kubernetes:yaml/v2:ConfigGroup   cg
    │  ├─ kubernetes:core/v1:ConfigMap  cg-default/cm1
    │  └─ kubernetes:core/v1:ConfigMap  cg-foo/cm2

There's a further complication: what if the kind is non-existent because a CRD hadn't been applied yet. In that case, one cannot determine whether the kind is namespaced and thus whether to apply the default namespace.

@EronWright
Copy link
Contributor Author

EronWright commented Mar 12, 2024

I'm going to close this as 'won't fix' due to the complication of missing kinds. The resource name would be too volatile if it hinged on whether a CRD is registered. Note the similar logic here that is less volatile because it doesn't affect the resource identity.

See similar issue: #147

@EronWright EronWright closed this as not planned Won't fix, can't repro, duplicate, stale Mar 12, 2024
@pulumi-bot pulumi-bot reopened this Mar 12, 2024
@pulumi-bot

This comment was marked as resolved.

@EronWright EronWright added the resolution/wont-fix This issue won't be fixed label Mar 12, 2024
@EronWright EronWright closed this as not planned Won't fix, can't repro, duplicate, stale Mar 12, 2024
@EronWright
Copy link
Contributor Author

EronWright commented Mar 19, 2024

I feel we need to reopen this issue, to make the implicit/explicit reordering logic work with objects in the default namespace. For example, imagine that an object has a depends-on annotation: /namespaces/default/Pod/my-pod. Imagine that the Pod spec doesn't have a namespace field because it is relying on the provider's default namespace. The namespace must be made explicit before the dependency may be realized.

@EronWright EronWright reopened this Mar 19, 2024
EronWright added a commit that referenced this issue Mar 21, 2024
<!--Thanks for your contribution. See [CONTRIBUTING](CONTRIBUTING.md)
    for Pulumi's contribution guidelines.

    Help us merge your changes more quickly by adding more details such
    as labels, milestones, and reviewers.-->

### Proposed changes

This PR strengthens the normalization of the parsed Kubernetes objects
to better interoperate with #2881.
Specifically:
1. Applies the default namespace as necessary on each object, so that
they may be reliably targeted by the `depends-on` resource.
2. Stabilizes the resource id (e.g. `default/my-map` versus `my-map`).
3. Validates that the objects are well-formed more eagerly than during
`Register`.

For example, given the below manifest and assuming that the default
namespace is `default`, we would expect a `DependsOn` option from
`Certificate` to `Issuer`. Observe that `namespace` isn't explicitly set
on the issuer.

```yaml
name: issue-2870-cert-manager
runtime: yaml
resources:
  example:
    type: kubernetes:yaml/v2:ConfigGroup
    properties:
      yaml: |
        apiVersion: cert-manager.io/v1
        kind: Issuer
        metadata:
          name: test-selfsigned
        spec:
          selfSigned: {}
        ---
        apiVersion: cert-manager.io/v1
        kind: Certificate
        metadata:
          name: selfsigned-cert
          annotations:
            config.kubernetes.io/depends-on: cert-manager.io/namespaces/default/Issuer/test-selfsigned
        spec:
          dnsNames:
            - example.com
          secretName: selfsigned-cert-tls
          issuerRef:
            name: test-selfsigned
```

To apply the default namespace correctly, one must know whether a given
kind is namespace-scoped. The provider naturally uses the discovery
client, and also looks for a matching CRD amongst the parsed objects
(since they're not yet installed).

### Testing
New test cases were added for `Normalize`, and some test cases were
moved from `Register` because the validation is performed more eagerly.

New sub-tests were added for `IsNamespacedKind` to cover the local CRD
enhancement.

### Related issues (optional)

<!--Refer to related PRs or issues: #1234, or 'Fixes #1234' or 'Closes
#1234'.
Or link to full URLs to issues or pull requests in other GitHub
repositories. -->

Closes #2870
@pulumi-bot pulumi-bot added the resolution/fixed This issue was fixed label Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/yaml kind/bug Some behavior is incorrect or out of spec resolution/fixed This issue was fixed resolution/wont-fix This issue won't be fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants