cmd/containerboot: allow setting --accept-dns via TS_EXTRA_ARGS again #16129
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pull Request Revisions
✅ AI review completed for r3 HelpReact with emojis to give feedback on AI-generated reviews:
We'd love to hear from you—reach out anytime at team@review.ai. |
ghost
reviewed
May 29, 2025
|
In the testEnv struct on line 1554, the Port fields have comments indicating their purpose, but the comment format is inconsistent with Go conventions. Consider using complete sentences ending with a period for all field comments. |
17f7fbc to
58189fa
Compare
ghost
reviewed
May 29, 2025
In 1.84 we made 'tailscale set'/'tailscale up' error out if duplicate command line flags are passed. This broke some container configurations as we have two env vars that can be used to set --accept-dns flag: - TS_ACCEPT_DNS- specifically for --accept-dns - TS_EXTRA_ARGS- accepts any arbitrary 'tailscale up'/'tailscale set' flag. We default TS_ACCEPT_DNS to false (to make the container behaviour more declarative), which with the new restrictive CLI behaviour resulted in failure for users who had set --accept-dns via TS_EXTRA_ARGS as the flag would be provided twice. This PR re-instates the previous behaviour by checking if TS_EXTRA_ARGS contains --accept-dns flag and if so using its value to override TS_ACCEPT_DNS. Updates #16108 Signed-off-by: Irbe Krumina <irbe@tailscale.com>
58189fa to
6057b9a
Compare
ghost
reviewed
May 30, 2025
| // do this to preserve the previous behaviour where --accept-dns could be | ||
| // set either via TS_ACCEPT_DNS or TS_EXTRA_ARGS. | ||
| acceptDNS := cfg.AcceptDNS != nil && *cfg.AcceptDNS | ||
| tsExtraArgs, acceptDNSNew := parseAcceptDNS(cfg.ExtraArgs, acceptDNS) |
There was a problem hiding this comment.
Is --accept-dns the situation where this sort of clash could occur? Of course the function we've added here is quite specific, so I am wondering whether down the line we are going to have to add more functions like this. If this is the case, could we benefit from something more all encompassing?
There was a problem hiding this comment.
No, we shouldn't add more env vars that allow setting the same flags that already can be set via TS_EXTRA_ARGS/TS_TAILSCALED_EXTRA_ARGS
ChaosInTheCRD
approved these changes
May 30, 2025
irbekrm
added a commit
that referenced
this pull request
May 30, 2025
…#16129) In 1.84 we made 'tailscale set'/'tailscale up' error out if duplicate command line flags are passed. This broke some container configurations as we have two env vars that can be used to set --accept-dns flag: - TS_ACCEPT_DNS- specifically for --accept-dns - TS_EXTRA_ARGS- accepts any arbitrary 'tailscale up'/'tailscale set' flag. We default TS_ACCEPT_DNS to false (to make the container behaviour more declarative), which with the new restrictive CLI behaviour resulted in failure for users who had set --accept-dns via TS_EXTRA_ARGS as the flag would be provided twice. This PR re-instates the previous behaviour by checking if TS_EXTRA_ARGS contains --accept-dns flag and if so using its value to override TS_ACCEPT_DNS. Updates #16108 Signed-off-by: Irbe Krumina <irbe@tailscale.com> (cherry picked from commit 5b670eb)
irbekrm
added a commit
that referenced
this pull request
May 30, 2025
…#16129) (#16140) In 1.84 we made 'tailscale set'/'tailscale up' error out if duplicate command line flags are passed. This broke some container configurations as we have two env vars that can be used to set --accept-dns flag: - TS_ACCEPT_DNS- specifically for --accept-dns - TS_EXTRA_ARGS- accepts any arbitrary 'tailscale up'/'tailscale set' flag. We default TS_ACCEPT_DNS to false (to make the container behaviour more declarative), which with the new restrictive CLI behaviour resulted in failure for users who had set --accept-dns via TS_EXTRA_ARGS as the flag would be provided twice. This PR re-instates the previous behaviour by checking if TS_EXTRA_ARGS contains --accept-dns flag and if so using its value to override TS_ACCEPT_DNS. Updates #16108 (cherry picked from commit 5b670eb) Signed-off-by: Irbe Krumina <irbe@tailscale.com>
KevinLiang10
pushed a commit
that referenced
this pull request
Jun 3, 2025
…#16129) In 1.84 we made 'tailscale set'/'tailscale up' error out if duplicate command line flags are passed. This broke some container configurations as we have two env vars that can be used to set --accept-dns flag: - TS_ACCEPT_DNS- specifically for --accept-dns - TS_EXTRA_ARGS- accepts any arbitrary 'tailscale up'/'tailscale set' flag. We default TS_ACCEPT_DNS to false (to make the container behaviour more declarative), which with the new restrictive CLI behaviour resulted in failure for users who had set --accept-dns via TS_EXTRA_ARGS as the flag would be provided twice. This PR re-instates the previous behaviour by checking if TS_EXTRA_ARGS contains --accept-dns flag and if so using its value to override TS_ACCEPT_DNS. Updates #16108 Signed-off-by: Irbe Krumina <irbe@tailscale.com>
thirdeyenick
pushed a commit
to ninech/tailscale
that referenced
this pull request
Jul 2, 2025
…tailscale#16129) (tailscale#16140) In 1.84 we made 'tailscale set'/'tailscale up' error out if duplicate command line flags are passed. This broke some container configurations as we have two env vars that can be used to set --accept-dns flag: - TS_ACCEPT_DNS- specifically for --accept-dns - TS_EXTRA_ARGS- accepts any arbitrary 'tailscale up'/'tailscale set' flag. We default TS_ACCEPT_DNS to false (to make the container behaviour more declarative), which with the new restrictive CLI behaviour resulted in failure for users who had set --accept-dns via TS_EXTRA_ARGS as the flag would be provided twice. This PR re-instates the previous behaviour by checking if TS_EXTRA_ARGS contains --accept-dns flag and if so using its value to override TS_ACCEPT_DNS. Updates tailscale#16108 (cherry picked from commit 5b670eb) Signed-off-by: Irbe Krumina <irbe@tailscale.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In 1.84 we made 'tailscale set'/'tailscale up' error out if duplicate command line flags are passed, see #16108
This broke some container configurations as we have two env vars that can be used to set --accept-dns flag:
We default TS_ACCEPT_DNS to false (to make the container behaviour more declarative- the CLI flags expect an explicit 'false' to change state true -> false, rather than just being unset), which with the new restrictive CLI behaviour resulted in failure for users who had set --accept-dns via TS_EXTRA_ARGS as the flag would be provided twice.
(See the defaulting here)
This PR re-instates the previous behaviour by checking if TS_EXTRA_ARGS contains --accept-dns flag and if so using its value to override TS_ACCEPT_DNS.
Really this illustrates that we should ideally move away from the env vars/flag parsing mechanism eventually.