Add Named Location support #441
Conversation
alexwilcox9
force-pushed
the
named-locations
branch
from
34d86d9
to
b3a389b
Compare
There was a problem hiding this comment.
Hi @alexwilcox9, thanks for working on this! I've had a quick look through and I think this is the right approach.
To avoid conflicts, we'll be able to look at merging once the v2 changes are in, but I've left a few comments in the meantime. Feel free to leave this open and we'll rebase it closer to the time.
| "id": { | ||
| Type: schema.TypeString, | ||
| Computed: true, | ||
| }, |
There was a problem hiding this comment.
We don't need to include id in the schema, the plugin SDK manages it implicitly.
There was a problem hiding this comment.
Noted and removed
There was a problem hiding this comment.
@manicminer tests fail without this as a result of panic: Invalid address to set: []string{"id"} from what I can see- Unless the branch needs updating with the latest sdk
There was a problem hiding this comment.
Ah I see, you need to use the d.SetId() method rather than d.Set("id") / tf.Set(d, "id")
| Type: schema.TypeString, | ||
| }, | ||
| }, | ||
| "is_trusted": { |
There was a problem hiding this comment.
I'd probably just call this field trusted
| func namedLocationResourceCreate(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics { | ||
| if useMsGraph := meta.(*clients.Client).EnableMsGraphBeta; useMsGraph { | ||
| return namedLocationResourceCreateMsGraph(ctx, d, meta) | ||
| } | ||
| return tf.ErrorDiagF(nil, "azuread_conditional_access_policy does not support AAD Graph. Please enable `EnableMsGraphBeta`") | ||
| } | ||
|
|
||
| func namedLocationResourceUpdate(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics { | ||
| if useMsGraph := meta.(*clients.Client).EnableMsGraphBeta; useMsGraph { | ||
| return namedLocationResourceUpdateMsGraph(ctx, d, meta) | ||
| } | ||
| return tf.ErrorDiagF(nil, "azuread_conditional_access_policy does not support AAD Graph. Please enable `EnableMsGraphBeta`") | ||
| } | ||
|
|
||
| func namedLocationResourceRead(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics { | ||
| if useMsGraph := meta.(*clients.Client).EnableMsGraphBeta; useMsGraph { | ||
| return namedLocationResourceReadMsGraph(ctx, d, meta) | ||
| } | ||
| return tf.ErrorDiagF(nil, "azuread_conditional_access_policy does not support AAD Graph. Please enable `EnableMsGraphBeta`") | ||
| } | ||
|
|
||
| func namedLocationResourceDelete(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics { | ||
| if useMsGraph := meta.(*clients.Client).EnableMsGraphBeta; useMsGraph { | ||
| return namedLocationResourceDeleteMsGraph(ctx, d, meta) | ||
| } | ||
| return tf.ErrorDiagF(nil, "azuread_conditional_access_policy does not support AAD Graph. Please enable `EnableMsGraphBeta`") | ||
| } |
There was a problem hiding this comment.
In v2, these shim functions will go away as we will no longer support AAD Graph
There was a problem hiding this comment.
Understood, once there's a PR for removing AAD support I'll refactor all of this to be inline with the new layout
| config := in[0].(map[string]interface{}) | ||
|
|
||
| ipRanges := config["ip_ranges"].([]interface{}) | ||
| isTrusted := config["is_trusted"].(interface{}) |
There was a problem hiding this comment.
This will already be returned as type interface{} so no need for the assertion here
There was a problem hiding this comment.
I've removed the last assertion there but left in the slice of interface assertion as that's the only way I could get the expand function to work
If there's a cleaner way to do it let me know and I'll give it another shot
alexwilcox9
force-pushed
the
named-locations
branch
from
b3a389b
to
683203c
Compare
|
Thanks for all the notes @manicminer Could you glance over it and see if there's anything blatantly wrong with it? Thanks in advance |
|
Currently trying to write some tests and the Import tests are hitting a Snag, not sure if its the way I've wrote the tests or if its in the read func although will review tommorow |
|
@kaovd could you try your test again? I think because it's an import I can't use the |
Sure will pull and retest. Also have you seen this for the ID param setting? #441 (comment) |
|
Ah I hadn't, thanks for that @manicminer do you think I should add a generic Get function to hamilton that works similar to the list function? |
that would probably make the exists check a whole lot easier tbf, in agreement with this. While it can be pulled via list a dedicated get function would be less intensive and a better solution. Also those two fixes did the trick on my tests, currently just got a basic test up as follows: my exists function is just checking for the IP right now. Without a generic get function I was thinking of just trying to get them both, see what exists and verify based off that but thats a bit more sloppy than a singular get func. For now im just going to return true and write a few more tests. Also I can see what you mean in regards to the issues with updating the ip array. Dosen't seem to be getting updated on MS's end might be a issue with the function |
|
Ok done some more debugging and found the problem, the Update functions in hamilton aren't setting the Odata types and therefore the request is ill formed although dosen't spit an error out. Whacking in In UpdateIp / UpdateCountry seems to fix this from what I can see. Will go make a PR. Post this I can request changes to add in my go tests or you can write your own whatevers good |
The Odata type functions are missed in the update functions which causes the requests to be ill formed and updates are not applied. See hashicorp/terraform-provider-azuread#441
|
Great spot @kaovd! |
|
I've had a go at writing some tests myself but the results are very inconsistent. This lines up with my experience of testing named locations in the hamilton client. It seems graph lags behind for whatever reason Also when I try to run all of the tests together |
| } | ||
|
|
||
| func TestAccNamedLocation_completeCountry(t *testing.T) { | ||
| data := acceptance.BuildTestData(t, "azuread_named_location", "test-country") |
There was a problem hiding this comment.
| data := acceptance.BuildTestData(t, "azuread_named_location", "test-country") | |
| data := acceptance.BuildTestData(t, "azuread_named_location", "test-complete-country") |
Here's the bug. Change this and I get
> $ TF_LOG=TRACE TF_ACC=1 go test -v -timeout=2h ./internal/services/namedlocations [±named-locations ●]
=== RUN TestAccNamedLocation_basicIP
=== PAUSE TestAccNamedLocation_basicIP
=== RUN TestAccNamedLocation_completeIP
=== PAUSE TestAccNamedLocation_completeIP
=== RUN TestAccNamedLocation_basicCountry
=== PAUSE TestAccNamedLocation_basicCountry
=== RUN TestAccNamedLocation_completeCountry
=== PAUSE TestAccNamedLocation_completeCountry
=== CONT TestAccNamedLocation_basicIP
=== CONT TestAccNamedLocation_completeCountry
=== CONT TestAccNamedLocation_basicCountry
=== CONT TestAccNamedLocation_completeIP
2021/06/02 19:23:29 [DEBUG] Importing Resource - parsing "0cc4a4a9-dfb3-425f-8ed1-70365cc7acdd"
2021/06/02 19:23:30 [DEBUG] Importing Resource - parsing "18b64bcf-75b4-40c0-865c-eecfb2016076"
2021/06/02 19:23:32 [DEBUG] Importing Resource - parsing "101afc99-0c08-48d7-b365-13fbf838d8bb"
2021/06/02 19:23:32 [DEBUG] Importing Resource - parsing "0dc61f25-134e-4bfe-853e-6db25f168dd6"
--- PASS: TestAccNamedLocation_basicIP (62.86s)
--- PASS: TestAccNamedLocation_basicCountry (62.96s)
--- PASS: TestAccNamedLocation_completeCountry (64.50s)
--- PASS: TestAccNamedLocation_completeIP (64.55s)
PASS
ok github.com/hashicorp/terraform-provider-azuread/internal/services/namedlocations 64.563s
Put two update tests in there for basic > complete on both ip / country and should be good.
There was a problem hiding this comment.
So for the update test do all of the test data sets have to have the same resource label? Looking at other test functions for other resources I see that all of the resource labels are "test" but when I switch over to this I start getting inconsistent errors
TF_LOG=TRACE TF_ACC=1 go test -v -timeout=2h ./internal/services/namedlocations
=== RUN TestAccNamedLocation_basicIP
=== PAUSE TestAccNamedLocation_basicIP
=== RUN TestAccNamedLocation_completeIP
=== PAUSE TestAccNamedLocation_completeIP
=== RUN TestAccNamedLocation_basicCountry
=== PAUSE TestAccNamedLocation_basicCountry
=== RUN TestAccNamedLocation_completeCountry
=== PAUSE TestAccNamedLocation_completeCountry
=== RUN TestAccNamedLocation_updateIP
=== PAUSE TestAccNamedLocation_updateIP
=== CONT TestAccNamedLocation_basicIP
=== CONT TestAccNamedLocation_completeCountry
=== CONT TestAccNamedLocation_basicCountry
=== CONT TestAccNamedLocation_completeIP
=== CONT TestAccNamedLocation_updateIP
2021/06/02 19:42:12 [DEBUG] Named Location with Object ID "02f3ea6a-9dac-4b36-930e-75e93e8c361a" was not found - removing from state
2021/06/02 19:42:12 [WARN] Got error running Terraform: exit status 1
Error: Provider produced inconsistent result after apply
When applying changes to azuread_named_location.test, provider
"provider[\"registry.terraform.io/hashicorp/azuread\"]" produced an
unexpected new value: Root resource was present, but now absent.
This is a bug in the provider, which should be reported in the provider's own
issue tracker.
=== CONT TestAccNamedLocation_completeIP
testcase.go:45: Step 1/2 error: Error running apply: exit status 1
Error: Provider produced inconsistent result after apply
When applying changes to azuread_named_location.test, provider
"provider[\"registry.terraform.io/hashicorp/azuread\"]" produced an
unexpected new value: Root resource was present, but now absent.
This is a bug in the provider, which should be reported in the provider's own
issue tracker.
--- FAIL: TestAccNamedLocation_completeIP (4.16s)
2021/06/02 19:42:12 [DEBUG] Named Location with Object ID "00b71689-e358-4489-8b13-1a88eb2654ef" was not found - removing from state
2021/06/02 19:42:12 [WARN] Got error running Terraform: exit status 1
Error: Provider produced inconsistent result after apply
When applying changes to azuread_named_location.test, provider
"provider[\"registry.terraform.io/hashicorp/azuread\"]" produced an
unexpected new value: Root resource was present, but now absent.
This is a bug in the provider, which should be reported in the provider's own
issue tracker.
=== CONT TestAccNamedLocation_basicIP
testcase.go:45: Step 1/2 error: Error running apply: exit status 1
Error: Provider produced inconsistent result after apply
When applying changes to azuread_named_location.test, provider
"provider[\"registry.terraform.io/hashicorp/azuread\"]" produced an
unexpected new value: Root resource was present, but now absent.
This is a bug in the provider, which should be reported in the provider's own
issue tracker.
--- FAIL: TestAccNamedLocation_basicIP (4.58s)
=== CONT TestAccNamedLocation_completeCountry
testcase.go:45: Step 1/2 error: Check failed: Check 1/1 error: running exists func for "azuread_named_location.test": Named Location with object ID "1ed56c5c-fd26-4097-b409-268a44e51865" does not exist
=== CONT TestAccNamedLocation_updateIP
testcase.go:45: Step 1/6 error: Check failed: Check 1/1 error: running exists func for "azuread_named_location.test": Named Location with object ID "092314fc-10d1-4311-a1ff-04af26a696bb" does not exist
2021/06/02 19:42:32 [DEBUG] Named Location with ID "092314fc-10d1-4311-a1ff-04af26a696bb" already deleted
2021/06/02 19:42:33 [DEBUG] Importing Resource - parsing "1c163a0b-c3a9-404c-bcf7-c841a4e80bdc"
testing_new.go:63: Error running post-test destroy, there may be dangling resources: "azuread_named_location.test" still exists
--- FAIL: TestAccNamedLocation_updateIP (44.98s)
--- FAIL: TestAccNamedLocation_completeCountry (45.37s)
--- PASS: TestAccNamedLocation_basicCountry (47.75s)
FAIL
FAIL github.com/hashicorp/terraform-provider-azuread/internal/services/namedlocations 47.761s
FAIL
|
See what you mean on inconsistency now - So the update tests are fine indivdually same with all others tests (although is some weirdness sometimes with MS Online rejecting conns or the remote breaking) but when I run together / after another test it starts complaining. Is graph rate limiting here? Oddly once I run things indivdually or a few times then go run them all together again its fine. |
|
Yup that lines up with what I've been seeing apart from the last bit; I haven't managed to get them all to run together successfully. When I compile the provider and actually use it it performs brilliantly. Will the tests passing inconsistently be a blocker for getting this merged post V2? |
|
I haven't run this yet - what is the actual API error? is it rate limiting proper, or perhaps related to replication delay? I've seen the latter a lot with named locations and hoping to have a workaround in the SDK soon. (We will need reliable tests before merging) |
|
@manicminer For me sometimes graph straight resets my conn but might be a me issue. The main issue has a likeness to this #407 actually. First test run will have that occur although if i run a few more times in different orders everything passes eventually. Could be multitude of problems? I've gone to test it some more and country tests are fine, all of a sudden IP tests are breaking for me with 500, however running the sample MS query from the docs in graph explorer does the same so guessing my environment is currently broke so until thats working not going to be able to keep testing. Not sure if itll just fix itself tommorow |
|
I'm wondering if something has improved on Microsoft's end now. Thought I'd just try running the tests again and have managed to successfully run them all concurrently five times over. |
|
Yep can confirm the tests are running fine off the bat for me too now as well. Not sure what MS where doing there. |
alexwilcox9
force-pushed
the
named-locations
branch
2 times, most recently
from
9e43c86
to
f1387a3
Compare
manicminer
force-pushed
the
named-locations
branch
from
1d20b11
to
32a67c5
Compare
|
@alexwilcox9 Thanks again for your early and continued work on this! 🚀 I rebased to resolve the conflicts, and added a few fixes. I used WaitForState funcs to smooth out the API delays when updating and deleting, and the tests are passing for me now. There's much inconsistency with this endpoint; after updating it will return both the old and new versions for awhile, and likewise after deleting it returns a mixture of 404 and 200 responses until the backend replication is seemingly completed. WaitForState has a useful Since my changes are substantial, I've pinged the rest of our team for further review. Test results:
|
| return namedLocationResourceRead(ctx, d, meta) | ||
| } | ||
|
|
||
| return tf.ErrorDiagF(errors.New("one of `ip` or `country` must be specified"), "Unable to determine named location type") |
There was a problem hiding this comment.
I would move the error message in an else branch of the if block and move the return statement outside of it.
There was a problem hiding this comment.
Good call, I played with this func so much I neglected to tidy that :)
| properties := expandIPNamedLocation(v.([]interface{})) | ||
| properties.BaseNamedLocation = &base | ||
|
|
||
| location, _, err := client.CreateIP(ctx, *properties) |
There was a problem hiding this comment.
I think it would be easier to understand this if location was renamed to ipLocation here and countryLocation in line 133.
|
This functionality has been released in v2.2.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Hi @manicminer
I'm trying to build on the great msgraph work you recently did to enable named locations support post V2.0 of the provider.
I've aimed to replicate the layout we decided on in hamilton by having one named location resource and then decide which functions to call based on its properties.
The code in this PR is far from ready but could you glance over it and let me know if this is the right direction to be going in or if I should be taking a completely different approach like two different resource types
Thanks!
Alex