Bound SA Token to Tekton Result API server in OpenShift #2145

khrm · 2024-05-09T01:32:16Z

This enables workload identity federation. We mount the projected the SA token at /var/run/secrets/openshift/serviceaccount

Changes

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Run make test lint before submitting a PR
Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Release Notes

Bound SA Token to Tekton Result API server Pod and mount it a`t /var/run/secrets/openshift/serviceaccount`.

khrm

/kind misc

tekton-robot · 2024-05-09T01:39:00Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/openshift/tektonresult/extension.go	12.2%	6.7%	-5.5

tekton-robot · 2024-05-09T01:46:31Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/openshift/tektonresult/extension.go	12.2%	41.3%	29.1

tekton-robot · 2024-05-09T05:36:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jkandasa

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~~ [jkandasa]

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

khrm · 2024-05-10T10:00:55Z

@savitaashture Probably, we can get this merge. Can you review this?

savitaashture · 2024-05-10T12:30:05Z

LGTM

pkg/reconciler/openshift/tektonresult/extension.go

piyush-garg · 2024-05-10T19:36:14Z

pkg/reconciler/openshift/tektonresult/extension.go

@@ -76,6 +84,7 @@ func (oe openshiftExtension) Transformers(comp v1alpha1.TektonComponent) []mf.Tr
 		occommon.RemoveRunAsUser(),
 		occommon.RemoveRunAsGroup(),
 		occommon.ApplyCABundles,
+		injectBoundSAToken(comp),


i think we should get the value of field required for this before and only pass the field required in transformer function

or add this function based on the condition

We already have a condition in injectBoundSAToken and we would need it anyway to apply only to a particular deployment. I feel having a condition introduces a bad coupling between functions.

So I am passing properties now.

pkg/reconciler/openshift/tektonresult/extension.go

piyush-garg · 2024-05-10T19:40:06Z

pkg/reconciler/openshift/tektonresult/extension.go

@@ -141,3 +150,79 @@ func filterAndTransform() client.FilterAndTransform {
 		return manifest, nil
 	}
 }
+
+// injectBoundSAToken adds a sa token projected volume to the Results Deployment
+func injectBoundSAToken(comp v1alpha1.TektonComponent) mf.Transformer {


this will be added for all the users. Can there will be cases where users do not want to do this?

Yes, it will be added for all users. It's a conscious decision on my part.
It doesn't matter whether the user requires this or not. This is something which is used when we have WIF. Even if it's not WIF, then we are just getting a SA token mounted.
We just don't allow removing this token which is provided by OpenShift in all clusters. Or we can have just a configuration to enable or disable this but every configuration adds overhead towards default installation.

@jkandasa if this looks good, i am fine

@khrm can you point some documentation about WIF will be available for all the namespaces?

from @khrm about WIF (I asked him on DM, What does WIF?)

Workload Identity Fedeation used by GCS and AWS. We support three format for logging atm: S3, GCS and PVC . Azure also has some identity management. We would need to support this later on. I didn't check how different it is from the rest.

https://docs.openshift.com/container-platform/4.11/authentication/managing_cloud_provider_credentials/cco-mode-sts.html

It's not required to be available at all namespaces. It's only for a particular deployment that needs cloud provider service like s3 or GCS bucket. The Kubernetes service account token needs to be available at "/var/run/secrets/openshift/serviceaccount".

In this case results API server requires s3 or GCS bucket for logging.

as discussed on chat, this will not impact and results should work fine for both type of usersm using WIF or not

tekton-robot · 2024-05-10T20:44:13Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/openshift/tektonresult/extension.go	12.2%	39.2%	27.0

This enables workload identity federation.

tekton-robot · 2024-05-13T08:47:38Z

The following is the coverage report on the affected files.
Say /test pull-tekton-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/openshift/tektonresult/extension.go	12.2%	39.2%	27.0

khrm · 2024-05-13T08:58:16Z

@piyush-garg Please re-review.

piyush-garg · 2024-05-13T13:16:04Z

/lgtm

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 9, 2024

tekton-robot requested review from piyush-garg and PuneetPunamiya May 9, 2024 01:32

khrm commented May 9, 2024

View reviewed changes

tekton-robot added kind/misc Categorizes issue or PR as a miscellaneuous one. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 9, 2024

khrm force-pushed the addBoundSATransformer branch from 8c5720f to d33c739 Compare May 9, 2024 01:39

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 9, 2024

jkandasa approved these changes May 9, 2024

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 9, 2024

savitaashture reviewed May 10, 2024

View reviewed changes

pkg/reconciler/openshift/tektonresult/extension.go Show resolved Hide resolved

savitaashture reviewed May 10, 2024

View reviewed changes

pkg/reconciler/openshift/tektonresult/extension.go Outdated Show resolved Hide resolved

piyush-garg reviewed May 10, 2024

View reviewed changes

pkg/reconciler/openshift/tektonresult/extension.go Show resolved Hide resolved

piyush-garg reviewed May 10, 2024

View reviewed changes

khrm force-pushed the addBoundSATransformer branch from d33c739 to 9076bdb Compare May 10, 2024 20:38

Bound SA Token to Tekton Result API server in OpenShift

893ae13

This enables workload identity federation.

khrm force-pushed the addBoundSATransformer branch from 9076bdb to 893ae13 Compare May 13, 2024 08:37

tekton-robot assigned piyush-garg May 13, 2024

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 13, 2024

tekton-robot merged commit 9915797 into tektoncd:main May 13, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bound SA Token to Tekton Result API server in OpenShift #2145

Bound SA Token to Tekton Result API server in OpenShift #2145

khrm commented May 9, 2024 •

edited

khrm left a comment

tekton-robot commented May 9, 2024

tekton-robot commented May 9, 2024

tekton-robot commented May 9, 2024

khrm commented May 10, 2024

savitaashture commented May 10, 2024

piyush-garg May 10, 2024

piyush-garg May 10, 2024

khrm May 10, 2024

khrm May 10, 2024

piyush-garg May 10, 2024 •

edited

khrm May 10, 2024

piyush-garg May 13, 2024

jkandasa May 13, 2024

khrm May 13, 2024

khrm May 13, 2024

khrm May 13, 2024

piyush-garg May 13, 2024

tekton-robot commented May 10, 2024

tekton-robot commented May 13, 2024

khrm commented May 13, 2024

piyush-garg commented May 13, 2024

Bound SA Token to Tekton Result API server in OpenShift #2145

Bound SA Token to Tekton Result API server in OpenShift #2145

Conversation

khrm commented May 9, 2024 • edited

Changes

Submitter Checklist

Release Notes

khrm left a comment

Choose a reason for hiding this comment

tekton-robot commented May 9, 2024

tekton-robot commented May 9, 2024

tekton-robot commented May 9, 2024

khrm commented May 10, 2024

savitaashture commented May 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piyush-garg May 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented May 10, 2024

tekton-robot commented May 13, 2024

khrm commented May 13, 2024

piyush-garg commented May 13, 2024

khrm commented May 9, 2024 •

edited

piyush-garg May 10, 2024 •

edited