Feat: Upgrade 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.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Upgrade 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

[k8s-ci-robot]

New changes are detected. LGTM label has been removed.

[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`

[VannTen]

Not that you can use the assert module for all these checks

[tico88612]

Do you mean like this?

- name: "External OCI Cloud Controller Manager | Credentials Check | external_oci_auth_key"
  ansible.builtin.assert:
    that:
      - not oci_use_instance_principals
      - external_oci_auth_key is not defined or not external_oci_auth_key
    fail_msg: "external_oci_auth_key is missing"

[VannTen]

Also, if the check are not conditional, maybe you can add all put them in one assert ? (since it accepts multiples statement).

We still get the assert in the error message, I figure it's as good as "is missing" wdty ?

[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.