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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add awaiter for service-account-token secret #2048

Merged
merged 4 commits into from
Jun 30, 2022
Merged

Add awaiter for service-account-token secret #2048

merged 4 commits into from
Jun 30, 2022

Conversation

kirecek
Copy link
Contributor

@kirecek kirecek commented Jun 27, 2022

Hi,

tbh this does not feel very right to me but still, I'd like to get your opinion since I already wrote the patch 馃槙

Proposed changes

Add watcher for V1Secret with the secret type of kubernetes.io/service-account-token.

i.e After the creation of serviceaccount token secret, pulumi seems to not have a native way of retrieving secret data which are filled by kubernetes controller. For example

const secret = new k8s.core.v1.Secret("example", {
    metadata: {
        name: "example",
        annotations: {
            "kubernetes.io/service-account.name": "default",
        },
    },
    type: "kubernetes.io/service-account-token",
});

where data from the secret secret.data.apply(v => v["token"]); will be undefined so users cannot work with its value.

The watcher simply waits for a secret to populate data which is immediate action.

Use-cases

Create access to API servers to services in multiple cluster - which is common practice for example in service meshes (istio, linkerd) but also in other tools that do some kind of similar federation.

Alternatives

1.) Use kubernetes client directly but TBH this is kinda repellent for new pulumi users

2.) Always read resources from the API after creation to fetchlive objects. But this change seems to be complex at first sight.

3.) There is a proposed generic watcher for user-specified resources #1260 but if we won't get any events I'm not sure it would solve this particular case. Although the issue is still open so not sure 馃し馃徎

WDYT?

@github-actions
Copy link

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

1 similar comment
@github-actions
Copy link

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

@lblackstone
Copy link
Member

/run-acceptance-tests

Copy link
Member

@lblackstone lblackstone left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @kirecek! 馃帀

I wasn't aware of this use case previously, but I think the approach looks good. I'd like to get an integration test added to cover the service-account-token case before merging. The test could create the secret and then export the value of .data and assert that the value is set.

This example is pretty close to what you'd need for this.

Let me know if you need any help figuring that part out.

provider/pkg/await/awaiters.go Show resolved Hide resolved
provider/pkg/await/awaiters.go Outdated Show resolved Hide resolved
@pulumi-bot
Copy link
Contributor

@github-actions
Copy link

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

@kirecek
Copy link
Contributor Author

kirecek commented Jun 30, 2022

Thank you. I tried to add a simple test based on the example you linked.

@lblackstone
Copy link
Member

/run-acceptance-tests

@pulumi-bot
Copy link
Contributor

Signed-off-by: Erik Jankovi膷 <erik.jankovic@gmail.com>
@github-actions
Copy link

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

@lblackstone
Copy link
Member

/run-acceptance-tests

@pulumi-bot
Copy link
Contributor

Copy link
Member

@lblackstone lblackstone left a comment

Choose a reason for hiding this comment

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

Looks like you just fixed the lint issue, so LGTM if it passes on the rerun! Thanks for the fix :)

Co-authored-by: Levi Blackstone <levi@pulumi.com>
@github-actions
Copy link

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

@lblackstone
Copy link
Member

/run-acceptance-tests

@pulumi-bot
Copy link
Contributor

@github-actions
Copy link

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

@lblackstone
Copy link
Member

/run-acceptance-tests

@pulumi-bot
Copy link
Contributor

@github-actions
Copy link

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

@lblackstone lblackstone merged commit 4f26021 into pulumi:master Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants