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

Pick a single, watched namespace for leader lock #278

Merged
merged 2 commits into from
May 20, 2022

Conversation

squaremo
Copy link
Contributor

@squaremo squaremo commented May 16, 2022

Proposed changes

At present when the environment variable WATCH_NAMESPACE is given, it is used verbatim as the namespace for the leadership election module; but, the value can be a comma-delimited list, e.g., foo,bar, which are not valid namespace names.

This commit fixes that by ensuring a single namespace name is used. Any of the watched namespaced would do, on the assumption that no other deployment of this operator will be using those namespaces.

Related issues (optional)

Fixes #273.

TODO

  • [ ] Unit tests (needs scaffolding, first) (EDIT: deprecated in favour of actually trying it)

At present when the environment variable `WATCH_NAMESPACE` is given,
it is used verbatim as the namespace for the leadership election
module; but, the value can be a comma-delimited list, e.g., `foo,bar`,
which are not valid namespace names.

This commit fixes that by ensuring a single namespace name is
used. Any of the watched namespaced would do, on the assumption that
no other deployment of this operator will be using those namespaces.

Signed-off-by: Michael Bridgen <mikeb@squaremobius.net>
@github-actions
Copy link

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

@lblackstone
Copy link
Member

/run-acceptance-tests

@github-actions
Copy link

@viveklak
Copy link
Contributor

Thanks for the PR @squaremo! Could you add a changelog entry for this?

@squaremo
Copy link
Contributor Author

squaremo commented May 20, 2022

I tried some scenarios by hand:

WATCH_NAMESPACE outcome expected?
fieldRef to metadata.namespace operator reacts to stack resources in default yes
"foo" ongoing error logged since no permissions for stack or configmap (for the lease) in namespace foo yes; indicates that it's trying to watch ns foo (you would need to adjust the RBAC to get it to work)
"default,foo" lease successfully goes in default, but repeated errors about not having permission for stack in foo (like above) yes, as for above
" , ," crashloop with log message yes, by design it should crash if WATCH_NAMESPACE can't be used
missing crashloop with log message about requiring WATCH_NAMESPACE unclear, I would expect it to watch all namespaces -- is this an operator-framework convention?
"" cannot list resource "stacks" in API group "pulumi.com" at the cluster scope unclear, I would expect that it watches all namespaces; maybe this is another operator-framework convention. In any case, I would not expect it to attempt to watch cluster-level resources (but perhaps I'm misinterpreting the error)

Is this acceptable for this PR @viveklak @lblackstone ? I'll write a changelog entry and move it out of draft, if so. Thanks for the reviews 🙏

@lblackstone
Copy link
Member

LGTM -- thanks for the detailed notes. Separate from this PR, I'd suggest opening an issue to track the last two test cases you mentioned (missing or empty). I also would have expected it to watch all namespaces by default.

Signed-off-by: Michael Bridgen <mikeb@squaremobius.net>
@github-actions
Copy link

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

@squaremo squaremo marked this pull request as ready for review May 20, 2022 16:35
@lblackstone lblackstone merged commit 71326e6 into pulumi:master May 20, 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.

Leader election breaks when multiple namespaces are specified
3 participants