Add day 1 API for cluster

[petrkotas]

Creates the initial implementation of the hcpCluster
API for day 1 operations.

[UlrichSchlueter]

Do we need the .devcontainer folder and the top level files here (README, .gitignore,.editorconfig) ?

[petrkotas]

@UlrichSchlueter we do not, it was remaining from the dependency of a different branch. I rebased and now the commit is out.

[mbarnes]

Dumb question on file structure, potentially more for myself than this PR since I'm coding these APIs in Go...

For comparison, ARO-RP puts API specifications under /swagger and the Go files under /pkg/api (not /api).

Do we want the TypeSpec files and Go files to live together under /api or would it be better to separate them, say by having these specs in a /typespec folder and the Go files in /api (or /pkg/api)?

[mbarnes]

This should be plural since it's an array, yes? (cf. ingressProfiles above)

[mbarnes]

Sorry for another one-off comment.

In the TypeSpec file I'm seeing a lot of

@visibility("create", "update")

As I understand TypeSpec visibility, it's converted directly to the x-ms-mutability OpenAPI annotation and "create, update" alone specifies a write-only field which ARM guidelines forbid:

OAPI027: API must not have write-Only properties
Write-only properties are not acceptible ("x-ms-mutability": ["create", "update"]).
User must be able to read the properties (GET/LIST).
This will create noise in what-if reports that would show write-only properties as an update.

I think these cases all need to be

@visibility("read", "create", "update")

which is equivalent to omitting @visibility.

[mbarnes]

Should this be named etcdEncryptionProfile for consistency's sake?

stale

[mbarnes]

Should discStorageAccountType be an enum?

[vkareh]

This is no longer used, since the etcdEncryptionSetId is now part of the PlatformProfile model.

[s-amann]

@petrkotas please rename the directory redhatopenshift/Hypershift to redhatopenshift/hcpcluster or redhatopenshift/hcpopenshiftcluster. There are also lots of "Hypershift" mentioned in the comments which needs to be refactored.

[mbarnes]

This field strikes me as redundant. Could we either drop it or make it read-only and just say the presence of a non-empty platform.etcdEncryptionSetId in the PUT request implies encryption is enabled?

[vkareh]

The reason we use this is to provide fast-fail validation. For example, if you set FIPS=true, we force etcdEncryption to also be true, which forces the request to fail if encryptionSetId is empty. There might be other use cases (at least we have some others in CS why we do it this way) that may or may not apply.

[mbarnes]

Presumably enforcing this consistency will be part of the RP's static validation. Still seems to me this field could be read-only; the static validation can check if etcdEncryptionSetId is non-empty as easily as whether etcdEncryption is true. What you're describing wrt FIPS sound like an implementation detail.

[petrkotas]

@s-amann I have updated the references to mention only hcpCluster.

[petrkotas]

I have also removed all links to openshift documentation as it was pointing to fixed version.

[mjlshen]

/lgtm