Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MI/WI - Generate secrets for platform identities #3802

Merged

Conversation

tsatam
Copy link
Collaborator

@tsatam tsatam commented Aug 28, 2024

Which issue this PR addresses:

Fixes ARO-9924

What this PR does / why we need it:

  • Adds a SecretLocation coordinate to PlatformWorkloadIdentityRole, that holds a NamespacedName reference to the default Secret location for each platform workload identity. These coordinates are intended for use internally, and will not be exposed on the public ARO API.
  • Adds a function generatePlatformWorkloadIdentitySecrets to the cluster manager for eventual use during ARO cluster installation (to generate secrets to pass to the installer) and during ARO updates (to directly update secrets on-cluster). Note that the current implementation as-is is unused anywhere within the codebase, and will be used in eventual work items ARO-4518 (create) and ARO-4371 (update).

Test plan for issue:

  • Unit tests were added for this direct functionality and they pass

Is there any documentation that needs to be updated for this PR?

No

How do you know this will function as expected in production?

This implementation as-is goes unused in any production contexts. It will eventually be utilized in MIWI cluster installation and update flows (see above).

@tsatam tsatam added the hold Hold label Aug 28, 2024
@tsatam tsatam changed the title Tsatam/hotfix miwi generate secrets for platform identities MI/WI - Generate secrets for platform identities Aug 28, 2024
@tsatam tsatam added chainsaw Pull requests or issues owned by Team Chainsaw loki ready-for-review labels Aug 28, 2024
@tsatam tsatam force-pushed the tsatam/hotfix-miwi-generate-secrets-for-platform-identities branch 3 times, most recently from 3874a34 to 49d7921 Compare August 28, 2024 22:22
@tsatam
Copy link
Collaborator Author

tsatam commented Aug 28, 2024

/azp run ci

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tsatam
Copy link
Collaborator Author

tsatam commented Aug 29, 2024

/azp run ci

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tsatam
Copy link
Collaborator Author

tsatam commented Aug 29, 2024

/azp run e2e

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tsatam
Copy link
Collaborator Author

tsatam commented Aug 29, 2024

/azp run ci,e2e

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@rajdeepc2792
Copy link
Collaborator

rajdeepc2792 commented Aug 29, 2024

LGTM, just a comment on additional manifests needed.
There are 2 more manifests:-

apiVersion: config.openshift.io/v1
kind: Authentication
metadata:
  name: cluster
spec:
  serviceAccountIssuer: <OIDC ISSUER URL>
---
apiVersion: v1
stringData:
  azure_tenant_id: xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx
kind: Secret
metadata:
  name: azure-credentials
  namespace: openshift-cloud-credential-operator
type: Opaque

Can/Should we handle these two at the RP side and in the same PR?

@tsatam
Copy link
Collaborator Author

tsatam commented Aug 29, 2024

LGTM, just a comment on additional manifests needed. There are 2 more manifests:-

apiVersion: config.openshift.io/v1
kind: Authentication
metadata:
  name: cluster
spec:
  serviceAccountIssuer: <OIDC ISSUER URL>
---
apiVersion: v1
stringData:
  azure_tenant_id: xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx
kind: Secret
metadata:
  name: azure-credentials
  namespace: openshift-cloud-credential-operator
type: Opaque

Can/Should we handle these two at the RP side and in the same PR?

For the Authentication resource, if it's a static, single resource as it appears to be, I think it can be done with a similar pattern to what's done here. It doesn't need to happen directly in this PR, but I can include it if it helps move our dependent work items along. Do we need to update this resource during cluster update as well (can the OIDC issuer URL change)?

For the CCO secret - I thought cloud-credential-operator would be included as a required platform workload identity and therefore have an entry in the roleset, meaning this PR would generate that secret with coordinates to a dedicated identity, is that not the case? Running oc adm release extract --from=${RELEASE_IMAGE} --credentials-requests --cloud azure gives me a CredentialsRequest resource for CCO so I assumed it'd get a dedicated identity.

@rajdeepc2792
Copy link
Collaborator

rajdeepc2792 commented Aug 29, 2024

Do we need to update this resource during cluster update as well (can the OIDC issuer URL change)?

No OIDC issuer URL update is not in scope currently, but the ensure function for the same can help here?

For the CCO secret - I thought cloud-credential-operator would be included as a required platform workload identity and therefore have an entry in the roleset, meaning this PR would generate that secret with coordinates to a dedicated identity, is that not the case? Running oc adm release extract --from=${RELEASE_IMAGE} --credentials-requests --cloud azure gives me a CredentialsRequest resource for CCO so I assumed it'd get a dedicated identity.

Tried executing oc adm release extract --cloud=azure --credentials-requests quay.io/openshift-release-dev/ocp-release:4.14.0-rc.6-x86_64 and it doesn't return the credential requests for CCO. The secret for CCO will not be part of the dedicated required Identities and secret will need to be created explicitly, as CCO doesn't require to access Azure resources, but it uses the TenantID for some of it's operations, probably related to setting up the webhook.

@tsatam
Copy link
Collaborator Author

tsatam commented Aug 29, 2024

/azp run ci,e2e

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@tsatam
Copy link
Collaborator Author

tsatam commented Aug 29, 2024

/azp run ci,e2e

@rajdeepc2792
Copy link
Collaborator

LGTM, since it is dependent on #3619, whenever it is merged this can be rebased and unblocked.

@tsatam
Copy link
Collaborator Author

tsatam commented Aug 29, 2024

/azp run ci,e2e

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@tsatam
Copy link
Collaborator Author

tsatam commented Aug 30, 2024

/azp run ci,e2e

cadenmarchese
cadenmarchese previously approved these changes Sep 11, 2024
Copy link
Collaborator

@rajdeepc2792 rajdeepc2792 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asked for changes related to TypeMeta when returning manifests.

This is needed in order to easily serialize these resources to YAML,
e.g. when setting them as string values in a Secret map for Hive to use
as an install manifest. Not setting these values will result in them being
omitted from the resulting JSON/YAML.
@tsatam
Copy link
Collaborator Author

tsatam commented Sep 12, 2024

/azp run ci,e2e

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@tsatam tsatam merged commit 73bc5f6 into master Sep 13, 2024
24 checks passed
@tsatam tsatam deleted the tsatam/hotfix-miwi-generate-secrets-for-platform-identities branch September 13, 2024 14:04
edisonLcardenas pushed a commit that referenced this pull request Sep 16, 2024
* Add secret location to PlatformWorkloadIdentityRoleSet

* Add generatePlatformWorkloadIdentitySecrets function

* Add mutable:true validate:required struct tags to SecretLocation fields on admin api

* Add functions for other required WI resources

* Remove redundant UsesWorkloadIdentity check from generatePlatformWorkloadIdentitySecrets

* Fix coordinates for static CCO secret; move static coordinate strings to const values

* Return resources as map (w/ filename as key) instead of list

* Explicitly set TypeMeta on workload identity resources

This is needed in order to easily serialize these resources to YAML,
e.g. when setting them as string values in a Secret map for Hive to use
as an install manifest. Not setting these values will result in them being
omitted from the resulting JSON/YAML.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chainsaw Pull requests or issues owned by Team Chainsaw loki ready-for-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants