feat!: support oauth client for the authkey#89
Conversation
📝 WalkthroughWalkthroughThis pull request introduces support for Tailscale OAuth clients as an authentication mechanism alongside existing tailnet keys. The change consolidates four individual authentication-related variables (ephemeral, expiry, preauthorized, reusable) into a single Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested Reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
44-57:⚠️ Potential issue | 🟡 MinorREADME example uses removed variable
ephemeral— will confuse users and fail on apply.The
ephemeralvariable was removed in this PR and replaced byauthkey_config. The example on line 56 still passesephemeral = true, which will produce an "unsupported argument" error for anyone copying this snippet. Please update the example to useauthkey_config, showing both OAuth client and tailnet key variants.📝 Suggested example update (tailnet key path, matching old defaults + ephemeral)
advertise_routes = [module.vpc.vpc_cidr_block] - ephemeral = true + authkey_config = { + tailscale_tailnet_key = { + ephemeral = true + expiry = 7776000 + preauthorized = true + reusable = true + } + } }Consider adding a second example block (or a comment) showing the OAuth client usage as well, since that's the main feature of this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 44 - 57, The README snippet still uses the removed variable ephemeral; update the example for module "tailscale" to replace ephemeral with the new authkey_config form and include two sample variants: one showing the tailnet key variant (equivalent to the old ephemeral behavior) and another showing the OAuth client variant; specifically, remove ephemeral = true, add an authkey_config block (e.g. with key = "<TAILNET_KEY>" or with oauth_client = { client_id = "...", client_secret = "...", redirect_uri = "..." } or similar fields used by the module) and include brief comments to indicate which block is which so users can copy the correct variant.
🧹 Nitpick comments (2)
variables.tf (2)
242-256: First validation block is redundant — the XOR check already implies "at least one".The second validation (lines 250-256) enforces exactly one of the two keys being non-null, which is a strict superset of the "at least one" check (lines 242-248). You can safely drop the first block. If you prefer keeping both for clearer error messages on different failure modes, that's fine too, but be aware the first can never trigger independently once the second exists.
♻️ Simplified to a single validation
- validation { - condition = ( - var.authkey_config.tailscale_oauth_client != null || - var.authkey_config.tailscale_tailnet_key != null - ) - error_message = "At least one of 'tailscale_oauth_client' or 'tailscale_tailnet_key' must be defined in authkey_config." - } - validation { condition = ( var.authkey_config.tailscale_oauth_client == null && var.authkey_config.tailscale_tailnet_key != null || var.authkey_config.tailscale_oauth_client != null && var.authkey_config.tailscale_tailnet_key == null ) - error_message = "Only one of 'tailscale_oauth_client' or 'tailscale_tailnet_key' must be defined in authkey_config." + error_message = "Exactly one of 'tailscale_oauth_client' or 'tailscale_tailnet_key' must be defined in authkey_config." }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@variables.tf` around lines 242 - 256, The first validation block checking var.authkey_config.tailscale_oauth_client != null || var.authkey_config.tailscale_tailnet_key != null is redundant because the second validation enforces an XOR (exactly one) and therefore already guarantees "at least one"; remove the first validation block (the one whose condition uses || and the error_message "At least one of 'tailscale_oauth_client' or 'tailscale_tailnet_key' must be defined in authkey_config.") so only the XOR validation remains, or if you prefer two messages keep both but ensure you don't duplicate logic—use the XOR validation in the block that references var.authkey_config.tailscale_oauth_client == null && var.authkey_config.tailscale_tailnet_key != null || var.authkey_config.tailscale_oauth_client != null && var.authkey_config.tailscale_tailnet_key == null to enforce exclusivity.
206-212: Inconsistent syntax in the default value — mix of:and=.Line 208 uses
:while line 211 uses=. Both are valid HCL, but mixing them in the same block is visually inconsistent and can confuse contributors.🧹 Proposed fix to unify syntax
default = { "tailscale_tailnet_key" = { - "ephemeral": false, - "expiry": 7776000, - "preauthorized": true, - "reusable" = true, + "ephemeral" = false + "expiry" = 7776000 + "preauthorized" = true + "reusable" = true } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@variables.tf` around lines 206 - 212, The default map for the "tailscale_tailnet_key" entry mixes HCL syntaxes (colon and equals); update the block so all attribute assignments use a single syntax style (e.g., change "ephemeral:", "expiry:", and "preauthorized:" to use "=" to match "reusable" or vice versa) within the default map for the tailscale_tailnet_key to make the declaration consistent and readable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main.tf`:
- Around line 18-21: The current authkey expression uses
try(one(tailscale_oauth_client.default[*].key),
one(tailscale_tailnet_key.default[*].key)) but try() returns the first value
even if it's null so the fallback never runs; modify the expression to use
coalesce for fallback semantics and keep try() as a safety net—i.e., call
coalesce(one(tailscale_oauth_client.default[*].key),
one(tailscale_tailnet_key.default[*].key)) inside try() so authkey resolves to
the first non-null key.
---
Outside diff comments:
In `@README.md`:
- Around line 44-57: The README snippet still uses the removed variable
ephemeral; update the example for module "tailscale" to replace ephemeral with
the new authkey_config form and include two sample variants: one showing the
tailnet key variant (equivalent to the old ephemeral behavior) and another
showing the OAuth client variant; specifically, remove ephemeral = true, add an
authkey_config block (e.g. with key = "<TAILNET_KEY>" or with oauth_client = {
client_id = "...", client_secret = "...", redirect_uri = "..." } or similar
fields used by the module) and include brief comments to indicate which block is
which so users can copy the correct variant.
---
Nitpick comments:
In `@variables.tf`:
- Around line 242-256: The first validation block checking
var.authkey_config.tailscale_oauth_client != null ||
var.authkey_config.tailscale_tailnet_key != null is redundant because the second
validation enforces an XOR (exactly one) and therefore already guarantees "at
least one"; remove the first validation block (the one whose condition uses ||
and the error_message "At least one of 'tailscale_oauth_client' or
'tailscale_tailnet_key' must be defined in authkey_config.") so only the XOR
validation remains, or if you prefer two messages keep both but ensure you don't
duplicate logic—use the XOR validation in the block that references
var.authkey_config.tailscale_oauth_client == null &&
var.authkey_config.tailscale_tailnet_key != null ||
var.authkey_config.tailscale_oauth_client != null &&
var.authkey_config.tailscale_tailnet_key == null to enforce exclusivity.
- Around line 206-212: The default map for the "tailscale_tailnet_key" entry
mixes HCL syntaxes (colon and equals); update the block so all attribute
assignments use a single syntax style (e.g., change "ephemeral:", "expiry:", and
"preauthorized:" to use "=" to match "reusable" or vice versa) within the
default map for the tailscale_tailnet_key to make the declaration consistent and
readable.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
doublethink13
force-pushed
the
oauth-client-support
branch
from
270455e to
c7efdaf
Compare
|
@doublethink13 please bump the Tailscale provider version constraint to 0.20.0 (first version with tailscale_oauth_client). |
done
you mean CodeRabbit nitpick comments? i've already addressed both of them 🙂 unless i'm missing something |
Ah, I might have been reviewing the old version. @doublethink13 thank you for your contribution! 🫶 |
gberenice
changed the title
happy to help! seems like an error reached main, this pr should fix it, but im not really sure why the tests are failing 😅 |
## what Fixing missing attribute in a variable default Fixing errors introduced in #89 #89 (comment) @gberenice can i have your review here please? <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Updated default configuration metadata for Subnet Router settings. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Veronika Gnilitska <veronika.gnilitska@gmail.com> Co-authored-by: Veronika Gnilitska <30597968+gberenice@users.noreply.github.com>
2 participants
what
tailscale upcommandwhy
references
Closes #86
Summary by CodeRabbit
Release Notes
New Features
Changes