Add integration for TAS #977
Conversation
lingyzhuang
requested review from
otaviof
and removed request for
jkopriva and
prietyc123
WalkthroughThe updates modify the TektonConfig Helm template to dynamically set the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/integrations/tas.go (2)
43-46: Inconsistent logging behavior for sensitive data.The logger logs the actual
rekorURLvalue but only the length oftufURL. For consistency and security, consider logging the length for both URLs or use a consistent approach for sensitive data logging.func (t *TASIntegration) log() *slog.Logger { return t.logger.With( "force", t.force, - "rekor-url", t.rekorURL, - "tuf-url", len(t.tufURL), + "rekor-url", len(t.rekorURL), + "tuf-url", len(t.tufURL), ) }
54-62: Consider more robust URL validation.The current validation only checks for the presence of
://which is minimal. Consider validating that the URLs are well-formed and use expected protocols (http/https).+import "net/url" func (t *TASIntegration) Validate() error { if t.rekorURL == "" { return fmt.Errorf("rekor-url is required") } - if !strings.Contains(t.rekorURL, "://") { - return fmt.Errorf("invalid rekor url, the protocol should be specified") + if _, err := url.Parse(t.rekorURL); err != nil { + return fmt.Errorf("invalid rekor url: %w", err) } if t.tufURL == "" { return fmt.Errorf("tuf url is required") } - if !strings.Contains(t.tufURL, "://") { - return fmt.Errorf("invalid tuf url, the protocol should be specified") + if _, err := url.Parse(t.tufURL); err != nil { + return fmt.Errorf("invalid tuf url: %w", err) } return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
installer/charts/tssc-pipelines/templates/tektonconfig/patch._tpl(2 hunks)installer/config.yaml(1 hunks)pkg/integrations/tas.go(1 hunks)pkg/subcmd/integration.go(1 hunks)pkg/subcmd/integration_tas.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
pkg/subcmd/integration.go (2)
pkg/subcmd/runner.go (1)
NewRunner(17-28)pkg/subcmd/integration_tas.go (1)
NewIntegrationTAS(64-87)
pkg/integrations/tas.go (4)
pkg/k8s/kube.go (1)
Kube(20-22)pkg/config/manager.go (1)
Name(26-26)pkg/k8s/secret.go (2)
SecretExists(27-40)DeleteSecret(43-54)pkg/integrations/common.go (1)
ErrSecretAlreadyExists(8-8)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Red Hat Konflux / rhtap-cli-on-pull-request
🔇 Additional comments (11)
pkg/integrations/tas.go (3)
80-87: Good design with lazy secret name generation.The lazy generation of the secret name ensures configuration is loaded before use. The hardcoded secret name "tssc-tas-integration" correctly matches the template consumption.
89-111: Proper secret existence handling with force flag support.The logic correctly handles existing secrets with appropriate error messaging and force flag behavior. The error wrapping with
ErrSecretAlreadyExistsfollows the established pattern frompkg/integrations/common.go.
113-145: Secure secret storage implementation.The secret creation follows Kubernetes best practices with proper type annotation, namespace assignment, and binary data encoding. The logging provides good observability without exposing sensitive data.
installer/config.yaml (1)
76-78: Logical dependency reordering for TAS integration.Moving
tssc-tasbeforetssc-pipelinesensures the TAS integration secret is available when the pipeline template references it. This aligns with the dynamic configuration changes in the Tekton template.pkg/subcmd/integration.go (1)
25-25: Consistent integration command registration.The TAS integration subcommand follows the established pattern and maintains proper alphabetical ordering in the command list.
installer/charts/tssc-pipelines/templates/tektonconfig/patch._tpl (2)
2-7: Robust dynamic configuration with proper fallbacks.The template correctly uses Helm's
lookupfunction to dynamically read therekor_urlfrom the TAS integration secret, with proper base64 decoding and graceful handling of missing secrets or data fields.
18-18: Appropriate fallback URL for missing secret.The fallback URL
http://rekor-server.tssc-tas.svcprovides a reasonable default that matches the expected service naming convention when the integration secret is not available.pkg/subcmd/integration_tas.go (4)
27-27: Proper interface compliance verification.The compile-time interface check ensures
IntegrationTAScorrectly implements theInterfacecontract, following Go best practices.
29-35: Clear and informative command description.The long description properly explains the command's purpose, credential storage mechanism, and target namespace, providing users with sufficient context.
42-60: Standard subcommand lifecycle implementation.The Complete/Validate/Run pattern follows the established convention with proper error handling and context propagation. The namespace creation before secret management is a good defensive practice.
64-87: Well-structured constructor with proper dependency injection.The constructor properly initializes all dependencies, sets up the Cobra command with appropriate metadata, and correctly binds the integration's persistent flags.
Roming22
left a comment
Roming22
left a comment
There was a problem hiding this comment.
The work on the integration might now be obsolete because of the cli focusing on demos/PoCs, but the explicit integration between Tekton and TAS is valuable IMO.
Please handle my comment, and we'll proceed with the merge.
lingyzhuang
force-pushed
the
new-tas-integration
branch
from
acb5473 to
ce64e0c
Compare
|
@Roming22 Removed related files. |
lingyzhuang
force-pushed
the
new-tas-integration
branch
from
ce64e0c to
c7bb9c0
Compare
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lingyzhuang, Roming22 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
openshift-merge-bot
bot
merged commit f661b91
into
redhat-appstudio:main
2 participants
transparency.urlto the value ofrekor_urlin integration secrettssc-tas-integrationJira: RHTAP-5258
Summary by CodeRabbit
New Features
Chores