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.

Sign up for GitHub

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

Jump to bottom

ROX-23709: Load token from file in fleetshard-sync #1802

Merged
merged 2 commits into from
Jun 3, 2024
Merged

ROX-23709: Load token from file in fleetshard-sync #1802

merged 2 commits into from
Jun 3, 2024

Conversation

kovayur
Copy link
Contributor

@kovayur kovayur commented May 8, 2024

Description

This change adds a new AuthType called TOKEN_FILE in order to load a service account token for authentication in Fleet Manager. This type becomes the default value in the dp-terraform helm chart instead of RHSSO.

Checklist (Definition of Done)

  • Unit and integration tests added
  • Added test description under Test manual
  • Documentation added if necessary (i.e. changes to dev setup, test execution, ...)
  • CI and all relevant tests are passing
  • Add the ticket number to the PR title if available, i.e. ROX-12345: ...
  • Discussed security and business related topics privately. Will move any security and business related topics that arise to private communication channel.
  • Add secret to app-interface Vault or Secrets Manager if necessary
  • RDS changes were e2e tested manually
  • Check AWS limits are reasonable for changes provisioning new resources
  • (If applicable) Changes to the dp-terraform Helm values have been reflected in the addon on integration environment

Test manual

TODO: Add manual testing efforts

# To run tests locally run:
make db/teardown db/setup db/migrate
make ocm/setup
make verify lint binary test test/integration

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented May 8, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added the approved label May 8, 2024
@kovayur kovayur requested a review from dhaus67 May 8, 2024 18:26
@kovayur kovayur force-pushed the yury/ROX-23709-fleetshard-authz branch from 0064307 to fd7a041 Compare May 10, 2024 19:17
@kovayur kovayur force-pushed the yury/ROX-23709-fleetshard-token-auth branch from 6629044 to b4fd1c0 Compare May 10, 2024 19:17
@kovayur
Copy link
Contributor Author

kovayur commented May 10, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @kovayur and the rest of your teammates on Graphite Graphite

This was referenced May 10, 2024
Merged
Merged
dhaus67
dhaus67 reviewed May 13, 2024
View reviewed changes
dp-terraform/helm/rhacs-terraform/templates/fleetshard-sync.yaml
projected:
sources:
- serviceAccountToken:
path: fleet-manager-token
Copy link

Choose a reason for hiding this comment

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

Let's also set a custom audience here (see also this comment) to have a) additional verification on FM side and b) avoid the token to be used to access the kube API.

dp-terraform/helm/rhacs-terraform/values.yaml Outdated
# Can be either OCM, RHSSO, STATIC_TOKEN. When choosing RHSSO, make sure the clientId/secret is set. By default, uses RHSSO.
authType: "RHSSO"
# Can be either OCM, RHSSO, STATIC_TOKEN, TOKEN_FILE. When choosing RHSSO, make sure the clientId/secret is set. By default, uses TOKEN_FILE.
authType: "TOKEN_FILE"
Copy link

Choose a reason for hiding this comment

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

nit: might be better to use service account token? It's a bit hard to really understand what is backing the "token file" auth type.

dp-terraform/helm/rhacs-terraform/templates/fleetshard-sync.yaml
@@ -148,3 +154,8 @@ spec:
- serviceAccountToken:
path: aws-token
audience: sts.amazonaws.com
- name: fleet-manager-token
Copy link

Choose a reason for hiding this comment

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

This should only be done optional if the auth type is used (same applies for the mount + env variable).

pkg/client/fleetmanager/impl/auth.go Outdated
@@ -94,6 +100,11 @@ func NewStaticAuth(ctx context.Context, opt StaticOption) (Auth, error) {
return newAuth(ctx, staticTokenFactory.GetName(), Option{Static: opt})
}

// NewFileAuth will return Auth that uses a token file to provide authentication for HTTP requests.
func NewFileAuth(ctx context.Context, opt TokenFileOption) (Auth, error) {
Copy link

Choose a reason for hiding this comment

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

nit: if we keep the name token file auth, this should be named appropriately then (i.e. NewTokenFileAuth).

pkg/client/fleetmanager/impl/auth_file.go Outdated

const (
// TokenFileAuthName is the name of the token file auth authentication method
TokenFileAuthName = "TOKEN_FILE"
Copy link

Choose a reason for hiding this comment

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

Doesn't need to be exposed I think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The constants of other types are public too.
It can be used in impl.NewAuth

pkg/client/fleetmanager/impl/auth_file.go Outdated
)

const (
// TokenFileAuthName is the name of the token file auth authentication method
Copy link

Choose a reason for hiding this comment

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

Suggested change
// TokenFileAuthName is the name of the token file auth authentication method
// TokenFileAuthName is the name of the token file auth authentication method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

pkg/client/fleetmanager/impl/auth_file.go Outdated
return TokenFileAuthName
}

// CreateAuth creates a new instance of Auth or returns error
Copy link

Choose a reason for hiding this comment

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

Suggested change
// CreateAuth creates a new instance of Auth or returns error
// CreateAuth creates a new instance of Auth or returns error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

pkg/client/fleetmanager/impl/auth_file.go Outdated
func (f *tokenFileAuthFactory) CreateAuth(_ context.Context, o Option) (Auth, error) {
tokenFile := o.TokenFile.File
if _, err := os.Stat(tokenFile); err != nil {
return nil, fmt.Errorf("failed to read token file %s: %w", tokenFile, err)
Copy link

Choose a reason for hiding this comment

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

Suggested change
return nil, fmt.Errorf("failed to read token file %s: %w", tokenFile, err)
return nil, fmt.Errorf("failed to read token file %q: %w", tokenFile, err)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

pkg/client/fleetmanager/impl/auth_file.go Outdated

// AddAuth add auth token to the request retrieved from OCM.
func (o *tokenFileAuth) AddAuth(req *http.Request) error {
token, err := shared.ReadFile(o.file)
Copy link

Choose a reason for hiding this comment

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

It's a bit unfortunate that we greedily always read from file here, especially since we do send quite a few requests from fleet-sync to fleet-manager.

On the options to improve this, we can either a) use an expiring cache where the duration is a bit less than the 1 hour b) use the file watcher concept we have in stackrox to read updates on changes.

Either way, I don't think we should be reading on every request here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Expiring cache added

pkg/client/fleetmanager/impl/testdata/token Outdated
@@ -0,0 +1 @@
some-value
Copy link

Choose a reason for hiding this comment

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

super nit: might make sense to just take the sample JWT from jwt.io and paste it here to have a more "closer to reality" example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@kovayur kovayur force-pushed the yury/ROX-23709-fleetshard-authz branch from d6f9367 to 2752423 Compare May 15, 2024 11:40
@kovayur kovayur force-pushed the yury/ROX-23709-fleetshard-token-auth branch from b4fd1c0 to e23cfa2 Compare May 15, 2024 11:44
@kovayur kovayur force-pushed the yury/ROX-23709-fleetshard-token-auth branch from e23cfa2 to 32edb7f Compare May 15, 2024 11:58
@kovayur kovayur force-pushed the yury/ROX-23709-fleetshard-authz branch from a7a9c4c to 44ede5a Compare May 15, 2024 11:59
@kovayur kovayur force-pushed the yury/ROX-23709-fleetshard-authz branch from 46ccc7b to bbf4f42 Compare May 16, 2024 12:47
@kovayur kovayur force-pushed the yury/ROX-23709-fleetshard-token-auth branch from 32edb7f to c350c3b Compare May 16, 2024 12:48
@kovayur kovayur force-pushed the yury/ROX-23709-fleetshard-token-auth branch from c350c3b to bb72437 Compare May 21, 2024 12:47
@kovayur kovayur force-pushed the yury/ROX-23709-fleetshard-authz branch from bbf4f42 to a40721e Compare May 21, 2024 12:48
@kovayur kovayur marked this pull request as ready for review May 21, 2024 14:52
@kovayur kovayur force-pushed the yury/ROX-23709-fleetshard-authz branch from 14eb2cd to c5a1af0 Compare May 22, 2024 10:14
@kovayur kovayur force-pushed the yury/ROX-23709-fleetshard-token-auth branch from 578da9a to c4e7dc6 Compare May 22, 2024 11:26
@kovayur kovayur force-pushed the yury/ROX-23709-fleetshard-authz branch from 6417b2b to f48b741 Compare May 22, 2024 15:17
Base automatically changed from yury/ROX-23709-fleetshard-authz to main May 22, 2024 16:06
@kovayur kovayur force-pushed the yury/ROX-23709-fleetshard-token-auth branch from c4e7dc6 to 4576c4b Compare May 22, 2024 16:26
@kovayur kovayur force-pushed the yury/ROX-23709-fleetshard-token-auth branch from 4576c4b to f54e96c Compare May 27, 2024 15:40
@kovayur kovayur force-pushed the yury/ROX-23709-fleetshard-token-auth branch from f54e96c to a0dc8e3 Compare May 27, 2024 16:11
@kovayur kovayur requested a review from dhaus67 May 28, 2024 12:42
@openshift-ci openshift-ci bot added the lgtm label May 29, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented May 29, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhaus67, kovayur

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:

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

@openshift-ci openshift-ci bot removed the lgtm label May 31, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented May 31, 2024

New changes are detected. LGTM label has been removed.

@kovayur kovayur force-pushed the yury/ROX-23709-fleetshard-token-auth branch from 96026d6 to 21275d8 Compare May 31, 2024 14:50
@kovayur
Copy link
Contributor Author

kovayur commented Jun 3, 2024

Had to rollback the default value in the helm chart due to the issues on the local / CI environment. Will follow-up in the next PR

@kovayur kovayur merged commit bcefc5f into main Jun 3, 2024
7 checks passed
@kovayur kovayur deleted the yury/ROX-23709-fleetshard-token-auth branch June 3, 2024 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhaus67 dhaus67 dhaus67 approved these changes

@ebensh ebensh Awaiting requested review from ebensh

@johannes94 johannes94 Awaiting requested review from johannes94

@GrimmiMeloni GrimmiMeloni Awaiting requested review from GrimmiMeloni

Assignees

@dhaus67 dhaus67

Labels
approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kovayur @dhaus67 @openshift-merge-robot