-
Notifications
You must be signed in to change notification settings - Fork 106
Cluster bot org auth #564
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
base: main
Are you sure you want to change the base?
Cluster bot org auth #564
Conversation
|
/retest-required |
|
/retest |
7515bb3 to
0847735
Compare
|
/label tide/merge-method-squash |
98956a7 to
01f559a
Compare
048be25 to
5ac51f6
Compare
|
/hold |
5ac51f6 to
516fd35
Compare
Makefile
Outdated
| # Use standard GO_BUILD_FLAGS for build tags (e.g., -tags gcs) | ||
| GO_BUILD_FLAGS ?= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After our standup, I took a look at this again and I think that you should probably just change this line to:
GO_BUILD_FLAGS=-tags gcs -trimpath
The current issue is that GO_BUILD_FLAGS is imported directly from build-machinery and using ?= doesn't work unless passed in on the command line. That's silly when we can force the issue ourselves and then we won't have to make any changes to openshift/release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each of us will have to update our IDE specific builds to add the tags accordingly, but for production we always produce the GCS version.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hoxhaeris 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 |
the index is not being built anymore in the service here, only consumed
update README to reflect the new changes (gcs) and link the new MD files
improve the auth data source, use a similar pluggable approach like in the orgdata-code; update documentation to reflect the auth config loading changes
update Makefile to use Openshift Builder
change the gcs bucket references
516fd35 to
41c9a9f
Compare
41c9a9f to
11c3508
Compare
…ands Extend authorization coverage to all commands that create or delete resources. Previously only launch, rosa create, workflow-launch, and mce create were protected. Commands now protected with authorization: - done: Deletes user's cluster resources - test upgrade: Creates test job resources - test: Creates test job resources - build: Creates build job resources - workflow-test: Creates test job resources - workflow-upgrade: Creates upgrade job resources - catalog build: Creates catalog build job resources - mce delete: Deletes MCE cluster resources Read-only commands remain unprotected: - list, auth, lookup, version, whoami (query only) - rosa lookup, rosa describe (query only) - mce auth, mce list, mce lookup (query only) - refresh (retries credentials fetch for existing cluster)
a723403 to
1e33ee3
Compare
|
@hoxhaeris: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| gcsBucket string | ||
| gcsObjectPath string | ||
| gcsProjectID string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add default values for these, since we know that they won't be changing. That way it keeps the command line from getting any more unwieldy.
| gcsObjectPath string | ||
| gcsProjectID string | ||
| gcsCredentialsJSON string | ||
| gcsCheckInterval time.Duration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment was specifically for the gcsCheckInterval variable
| if err := orgdata.SetupGCSDataSource(ctx, gcsConfig, orgDataService); err != nil { | ||
| klog.Warningf("GCS setup failed: %v. Running without authorization data (permit all mode)", err) | ||
| orgDataService = nil // Clear the service since we couldn't load any data | ||
| } | ||
|
|
||
| // Initialize authorization service if config is provided | ||
| if opt.authorizationConfigPath != "" { | ||
| klog.Infof("Initializing authorization service with config: %s", opt.authorizationConfigPath) | ||
| authService = orgdata.NewAuthorizationService(orgDataService, opt.authorizationConfigPath) | ||
| if err := authService.LoadConfig(ctx); err != nil { | ||
| klog.Warningf("Failed to load authorization config: %v", err) | ||
| // Keep the authService even if config fails to load - it will allow all commands | ||
| } else { | ||
| klog.Infof("Authorization service successfully initialized with config: %s", opt.authorizationConfigPath) | ||
| } | ||
|
|
||
| // Start config watcher regardless of initial load success - it will detect file creation | ||
| go func() { | ||
| if err := authService.StartConfigWatcher(ctx); err != nil { | ||
| klog.Infof("Authorization config watcher stopped: %v", err) | ||
| } | ||
| }() | ||
| } else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is no orgDataService, why bother setting up the authService at all?
| @@ -0,0 +1,103 @@ | |||
| # Authorization configuration for ci-chat-bot | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make sure that this is referenced as an "Example" or "Sample" file and not the real configuration (to avoid any confusion)
/cc @bradmwilliams
/cc @jupierce