Allow configuring RKE ACI network provider #912

fpants · 2022-04-28T13:44:42Z

Issue: #922

This PR adds logic for configuring RKE clusters with the ACI network provider.

The Rancher API already supports this (see https:///v3/schemas/aciNetworkProvider), so this is only to also expose this in the Rancher Terraform provider.

Similar ACI configuration is already available in the RKE Terraform provider: https://registry.terraform.io/providers/rancher/rke/latest/docs/resources/cluster#aci_network_provider

I'm not entirely sure everything is covered, so please make suggestions for any improvements.

a-blender

Please update the following

rancher2/schema_cluster_rke_config_network.go

docs/resources/cluster.md

fpants · 2022-05-17T06:52:01Z

Please update the following, otherwise looks great!

Thanks! I've updated the fields and the corresponding docs.

a-blender

Thanks for the changes! I see a whole bunch of fields here not listed in the rke_docs. Could you explain why they are added and if they aren't needed, remove them?

docs/resources/cluster.md

a-blender

LGTM, fields wise. We may need end-to-end acceptance tests for a basic implementation of the aci_network_provider field in the tf config for RKE clusters using the rancher2 provider. Merging this PR is pending some internal discussions on our end. Thank you for your patience.

fpants · 2022-05-25T12:03:58Z

Thanks! We have environments running with Rancher managing RKE clusters using the ACI CNI provider, and provisioning them using the Terraform provider with this code is working. I'd be happy to assist with end-to-end testing.

a-blender · 2022-06-29T13:20:09Z

Blocked by rancher/kontainer-driver-metadata#922. Waiting on all feedback comments to be addressed and tested before reviewing with another tf SME. Once that PR is merged then this one can be re-reviewed and merged for ACI support in the Terraform provider

a-blender · 2022-07-12T15:54:49Z

ACI support rancher/kontainer-driver-metadata#922 (KDM) and rancher/rke#2963 (rke) have been merged, unblocking

a-blender

Please add tf structure tests for aci_network_provider fields to make sure expansion and flatteners work correctly. Also provision an RKE cluster and import it into rancher with a simple ACI configuration to make sure that works. Then this will good to merge

a-blender · 2022-07-13T17:01:51Z

Apologies! Tests have already been added

The rke PR rancher/rke#2963 has added extra fields to CNI version 5.2.3.2 here https://github.com/rancher/rke/pull/2963/files#diff-d6d39102a5c2432600e3ffac3abe79885d842926704befe5bdb3063be71518b7R653-R663. Please add these fields to this PR and update the tests, thank you

fpants · 2022-07-14T14:09:31Z

Before I can add the fields, I think they also need to be added here: https://github.com/rancher/rancher/blob/release/v2.6/pkg/client/generated/management/v3/zz_generated_aci_network_provider.go
Otherwise they can't be referenced and result in compile errors like:

rancher2/structure_cluster_rke_config_network.go:21:12: in.ApicRefreshTickerAdjust undefined (type *"github.com/rancher/rancher/pkg/client/generated/management/v3".AciNetworkProvider has no field or method ApicRefreshTickerAdjust)

I'm not sure how those files are generated in the rancher/rancher repo, but according to the latest commit to the file, it's based on the rancher/rke go dependency. Perhaps the rke package needs to be released first?

a-blender · 2022-07-14T16:52:15Z

@fpants Correct, the tf rancher2 provider uses rancher, which uses rke under the hood. The ACI field support is not in a released version of rke yet so rancher doesn't know about them. Go ahead and add the fields/tests to this PR. I'll make sure the latest version of rke is released before this PR gets merged.

Merge blocked by rke v1.3.14 release https://github.com/rancher/rke/releases

fpants · 2022-07-15T10:50:12Z

Thanks @annablender, that makes sense. I have added the fields introduced in ACI-CNI 5.2.3.2.
I have also tested importing an existing RKE cluster with the ACI CNI installed, and it shows up and works in Rancher without issues.

a-blender · 2022-07-15T21:17:14Z

@fpants Thank you!

I see the build is failing

# github.com/rancher/terraform-provider-rancher2/rancher2 [github.com/rancher/terraform-provider-rancher2/rancher2.test]
112s
358	rancher2/structure_cluster_rke_config_network.go:21:11: in.ApicRefreshTickerAdjust undefined (type *"github.com/rancher/rancher/pkg/client/generated/management/v3".AciNetworkProvider has no field or method ApicRefreshTickerAdjust)
112s
359	rancher2/structure_cluster_rke_config_network.go:22:41: in.ApicRefreshTickerAdjust undefined (type *"github.com/rancher/rancher/pkg/client/generated/management/v3".AciNetworkProvider has no field or method ApicRefreshTickerAdjust)
112s
360	rancher2/structure_cluster_rke_config_network.go:27:11: in.ApicSubscriptionDelay undefined (type *"github.com/rancher/rancher/pkg/client/generated/management/v3".AciNetworkProvider has no field or method ApicSubscriptionDelay)
112s
361	rancher2/structure_cluster_rke_config_network.go:28:38: in.ApicSubscriptionDelay undefined (type *"github.com/rancher/rancher/pkg/client/generated/management/v3".AciNetworkProvider has no field or method ApicSubscriptionDelay)
112s
362	rancher2/structure_cluster_rke_config_network.go:45:11: in.DisablePeriodicSnatGlobalInfoSync undefined (type *"github.com/rancher/rancher/pkg/client/generated/management/v3".AciNetworkProvider has no field or method DisablePeriodicSnatGlobalInfoSync)
112s
363	rancher2/structure_cluster_rke_config_network.go:46:53: in.DisablePeriodicSnatGlobalInfoSync undefined (type *"github.com/rancher/rancher/pkg/client/generated/management/v3".AciNetworkProvider has no field or method DisablePeriodicSnatGlobalInfoSync)
112s
364	rancher2/structure_cluster_rke_config_network.go:48:11: in.DisableWaitForNetwork undefined (type *"github.com/rancher/rancher/pkg/client/generated/management/v3".AciNetworkProvider has no field or method DisableWaitForNetwork)
112s
365	rancher2/structure_cluster_rke_config_network.go:49:39: in.DisableWaitForNetwork undefined (type *"github.com/rancher/rancher/pkg/client/generated/management/v3".AciNetworkProvider has no field or method DisableWaitForNetwork)
112s
366	rancher2/structure_cluster_rke_config_network.go:54:11: in.DurationWaitForNetwork undefined (type *"github.com/rancher/rancher/pkg/client/generated/management/v3".AciNetworkProvider has no field or method DurationWaitForNetwork)
112s
367	rancher2/structure_cluster_rke_config_network.go:55:40: in.DurationWaitForNetwork undefined (type *"github.com/rancher/rancher/pkg/client/generated/management/v3".AciNetworkProvider has no field or method DurationWaitForNetwork)
112s
368	rancher2/structure_cluster_rke_config_network.go:55:40: too many errors
112s
369	FAIL	github.com/rancher/terraform-provider-rancher2/rancher2 [build failed]
128s
370	FAIL

This is happening because rancher does not have the additional ACI fields supported in the latest rke in its types structure. We are releasing rke v1.3.13-rc4 rancher/rancher#38295 by today. Once the CI passes here this will be ready for a final re-review and merge.

a-blender · 2022-08-16T18:26:14Z

RKE 1.3.14 has been merged rancher/rancher#38295 and started being used by rancher on July 15, which should fix the above types error. Dependencies also needed to be bumped. The PR for that #959 has also been merged. Waiting on CI... will ping reviewers when this passes

HarrisonWAffel

LGTM, build is passing

MKlimuszka requested a review from eliyamlevy May 10, 2022 15:50

a-blender requested review from a-blender, jakefhyde and snasovich May 11, 2022 18:10

a-blender suggested changes May 16, 2022

View reviewed changes

rancher2/schema_cluster_rke_config_network.go Show resolved Hide resolved

docs/resources/cluster.md Outdated Show resolved Hide resolved

fpants requested a review from a-blender May 17, 2022 13:24

a-blender suggested changes May 18, 2022

View reviewed changes

docs/resources/cluster.md Show resolved Hide resolved

docs/resources/cluster.md Show resolved Hide resolved

a-blender reviewed May 24, 2022

View reviewed changes

a-blender changed the title ~~Allow configuring RKE ACI network provider~~ [DNM] Allow configuring RKE ACI network provider May 24, 2022

a-blender requested review from a-blender, kinarashah and jakefhyde and removed request for jakefhyde July 12, 2022 15:56

a-blender reviewed Jul 12, 2022

View reviewed changes

fpants added 4 commits August 16, 2022 14:13

add rke aci network provider

60e3aff

document rke aci network provider

cb9a01b

specify required and sensitive rke aci network provider arguments

dd573a6

add fields introduced in ACI-CNI 5.2.3.2

d943312

a-blender force-pushed the feature/rke-aci-network-provider branch from c0705ee to d943312 Compare August 16, 2022 18:14

a-blender changed the title ~~[DNM] Allow configuring RKE ACI network provider~~ Allow configuring RKE ACI network provider Aug 16, 2022

a-blender self-requested a review August 16, 2022 18:14

a-blender requested review from HarrisonWAffel and removed request for snasovich August 16, 2022 18:15

a-blender mentioned this pull request Aug 16, 2022

Add RKE Support for ACI Network Provider to Terraform Provider #922

Closed

a-blender approved these changes Aug 16, 2022

View reviewed changes

HarrisonWAffel approved these changes Aug 16, 2022

View reviewed changes

a-blender merged commit d6b9660 into rancher:master Aug 17, 2022

jakefhyde mentioned this pull request Aug 30, 2022

Update CHANGELOG with 1.24.1 release notes #981

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow configuring RKE ACI network provider #912

Allow configuring RKE ACI network provider #912

fpants commented Apr 28, 2022 •

edited by a-blender

a-blender left a comment •

edited

fpants commented May 17, 2022

a-blender left a comment

a-blender left a comment •

edited

fpants commented May 25, 2022

a-blender commented Jun 29, 2022 •

edited

a-blender commented Jul 12, 2022

a-blender left a comment

a-blender commented Jul 13, 2022 •

edited

fpants commented Jul 14, 2022

a-blender commented Jul 14, 2022 •

edited

fpants commented Jul 15, 2022

a-blender commented Jul 15, 2022 •

edited

a-blender commented Aug 16, 2022 •

edited

HarrisonWAffel left a comment

Allow configuring RKE ACI network provider #912

Allow configuring RKE ACI network provider #912

Conversation

fpants commented Apr 28, 2022 • edited by a-blender

a-blender left a comment • edited

Choose a reason for hiding this comment

fpants commented May 17, 2022

a-blender left a comment

Choose a reason for hiding this comment

a-blender left a comment • edited

Choose a reason for hiding this comment

fpants commented May 25, 2022

a-blender commented Jun 29, 2022 • edited

a-blender commented Jul 12, 2022

a-blender left a comment

Choose a reason for hiding this comment

a-blender commented Jul 13, 2022 • edited

fpants commented Jul 14, 2022

a-blender commented Jul 14, 2022 • edited

fpants commented Jul 15, 2022

a-blender commented Jul 15, 2022 • edited

a-blender commented Aug 16, 2022 • edited

HarrisonWAffel left a comment

Choose a reason for hiding this comment

fpants commented Apr 28, 2022 •

edited by a-blender

a-blender left a comment •

edited

a-blender left a comment •

edited

a-blender commented Jun 29, 2022 •

edited

a-blender commented Jul 13, 2022 •

edited

a-blender commented Jul 14, 2022 •

edited

a-blender commented Jul 15, 2022 •

edited

a-blender commented Aug 16, 2022 •

edited