Feat: Add external OCI cloud controller manager

[tico88612]

What type of PR is this?

/kind feature

What this PR does / why we need it:

• Upgrade OCI cloud controller manager and unify the variable naming.
• In-tree cloud provider will remove at K8s v1.31. To avoid ambiguity, I suggest v2.27 would be better to remove roles/kubernetes-app/cloud-provider.
• Unified puts the external cloud controller manager into role/kubernetes-app/external_cloud_controller.

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Add external Oracle cloud infrastructure cloud controller manager

[yankay]

/ok-to-test

[yankay]

Thanks @tico88612

The release note maybe
”Add external Oracle cloud infrastructure cloud controller manager “
is better.

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tico88612, yankay

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [yankay]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ant31]

/lgtm

[tico88612]

/retest-required

[tico88612]

@VannTen, Could you help me with this PR? I want to remove the cloud provider from roles and call it Cloud Controller Manager. Thank you!

[VannTen]

Hum, OCI is somewhat ambiguous name ... is there a way to use which denotes clearly this is the oracle cloud infra controller ? I know the previous internal was named OCI, but since we're creating a new one anyway, maybe we could have a less confusing name ? (-> confusion with open container image)

For the content itself:
I've seen several check of the form : var is defined and var == something -> can we have the var in defaults instead or is there some things preventing that ?

Thanks

[tico88612]

Hum, OCI is somewhat ambiguous name ... is there a way to use which denotes clearly this is the oracle cloud infra controller ? I know the previous internal was named OCI, but since we're creating a new one anyway, maybe we could have a less confusing name ? (-> confusion with open container image)

OCI is an awkward acronym; it conflicts with the Open Container Initiative, and I don't have a better idea now.

Some of the code was migrated from roles/kubernetes-app/cloud-provider because the (in-tree) cloud-provider was removed to avoid confusion later. Incidentally, the Oracle cloud provider was upgraded.

For the content itself:
I've seen several check of the form : var is defined and var == something -> can we have the var in defaults instead or is there some things preventing that ?

Previously, the variables cloud_provider and external_cloud_provider were defined before executing the associated tasks. I was under the impression that there were anti-dumbing checks.

[VannTen]

OCI is an awkward acronym; it conflicts with the Open Container Initiative, and I don't have a better idea now.

Maybe oracle_oci ? Or oracle_cloud(_infra) ?

> For the content itself: > I've seen several check of the form : var is defined and var == something -> can we have the var in defaults instead or is there some things preventing that ? Previously, the variables `cloud_provider` and `external_cloud_provider` were defined before executing the associated tasks. I was under the impression that there were anti-dumbing checks.

I meant more stuf list `oci_security_lists or `external_oci_auth_user`

[tico88612]

I meant more stuf list oci_security_lists or external_oci_auth_user

I think giving a default value to the user setting is unnecessary because it's not a required option in Kubespray, and it's only triggered if external_cloud_provider is set to oci.

[VannTen]

It's not only about defaults, it's also about documentation. One of the goals is to use roles/defaults/*.yml as documentation ultimately, rather than the sample inventory. It also makes templates more readable (IMO) to not have the double checks (is defined + testing the value)

[tico88612]

It's not only about defaults, it's also about documentation. One of the goals is to use roles/defaults/*.yml as documentation ultimately, rather than the sample inventory. It also makes templates more readable (IMO) to not have the double checks (is defined + testing the value)

What do you mean? Do other user settings go to roles/kubernetes-apps/external_cloud_controller/oci/defaults/main.yml?

[VannTen]

Yes, they should.

[tico88612]

Wouldn't adding the user setting eliminate the need for roles/kubernetes-apps/external_cloud_controller/oci/tasks/oci-credential-check.yml? (except that the value check for external_oracle_load_balancer_security_list_management_mode must be [“All”, “Frontend”, “None”])

[VannTen]

Well if the default value ("" for most of them FWICS) is acceptable no need to test for it. On the other hand if it isn't you still need to assert.

[tico88612]

This method doesn't need to check whether the string is defined; it just needs to ensure it's not empty.

PTAL, thanks!

[ant31]

can you please update the release note as suggested @yankay

looks good thanks

[tico88612]

@ant31 updated.

[tico88612]

Can this be merged? This is related to #11633.

[VannTen]

@tico88612 I added a commit to make the assert more compact, what do you think ?

[tico88612]

@VannTen I think I learned a pretty good solution from you, and it's better to read, too. Thank you!

[VannTen]

Then
/lgtm