- Notifications
You must be signed in to change notification settings - Fork52
tailscale/resource_acl: relax policy validation during plan steps#554
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
docs/resources/acl.md Outdated
| ports = ["*:*"], | ||
| "src": ["*"], | ||
| "dst": ["*"], | ||
| "ip": ["*"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Drive-by, the HuJSON example was not valid. Switching from the ACL to the Grant syntax at the same time.
| // non-syntax errors are logged but do not fail the plan operation | ||
| tflog.Debug(ctx,"ACL validation unsuccessful due to advisory error",map[string]interface{}{"error":err}) | ||
| returnnil | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
From our conversations, warnings might be counterproductive and confuse people, but we should not make the information completely unavailable.
Some resources like SCIM groups can be created and added as a reference to the policy file in the same run. The eager validation causes otherwise valid runs to fail at the plan step because the references do not exist yet.We are relaxing the plan validation logic to avoid such false positives. The full validation results are still available in debug logs in case this is useful. Note that the validation is done in full during an apply before the policy file is changed so this does not permit a tailnet policy to be in an invalid state.Fixes#546Signed-off-by: mcoulombe <max@tailscale.com>
5e104df toc70357aCompare
oxtoacart left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Code LGTM, just a couple of suggestions.
| returnerr | ||
| }elseiferr!=nil { | ||
| // non-syntax errors are logged but do not fail the plan operation | ||
| tflog.Debug(ctx,"ACL validation unsuccessful due to advisory error",map[string]interface{}{"error":err}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| tflog.Debug(ctx,"ACL validation unsuccessful due to advisoryerror",map[string]interface{}{"error":err}) | |
| tflog.Debug(ctx,"ACL validation unsuccessful due to advisorywarning, apply may fail if dependencies are unmet",map[string]interface{}{"error":err}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I can soften the wording and call them warnings, but the validation failures are not necessarily undefined dependencies. The provider should not characterize what caused the error if it just forwards what the BE returned, which can change in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
are not necessarily
Hence the use of "may"
| // TODO: use an exported variable when https://github.com/hashicorp/terraform-plugin-sdk/issues/803 has been addressed. | ||
| constUnknownVariableValue="74D93920-ED26-11E3-AC10-0800200C9A66" | ||
| varsyntaxErrorMessage=regexp.MustCompile(`^ACL validation failed: line \d+, column \d+:`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe include a comment about what assumptions this makes about the control server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What do you mean by assumptions about the control server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The assumption that all syntax errors present as an error in this form, and that if this change, the logic breaks.
oxtoacart commentedOct 10, 2025
@mcoulombe Should we just close this one? |
mcoulombe commentedOct 10, 2025
Once I'm done with the WIF work my plan is to add error codes on the validate endpoint and use that code here instead of the error message format, it seems reasonable to me to update this PR once the BE is ready instead of opening a new one. |
mcoulombe commentedOct 14, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Converting to draft since it requires downstream BE changes and has been a bit deprioritized to get some internal projects over the finish line. I'll come back to it as soon when possible |
Uh oh!
There was an error while loading.Please reload this page.
What this PR does / why we need it:
Some resources like SCIM groups can be created and added as a reference to the policy file in the same run. The eager validation causes otherwise valid runs to fail at the plan step because the references do not exist yet.
We are relaxing the plan validation logic to avoid such false positives. The full validation results are still available in debug logs in case this is useful. Note that the validation is done in full during an apply before the policy file is changed so this does not bypass the full breath of validations taking place before the policy is modified.
Which issue this PR fixes(use
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged):Fixes#546
Example of a validation error for a failed test in the debug logs:
Resulting apply if the user ignores or does not check the validation logs before an apply: