Skip to content

feat(console): use securesign Rekor URL instead of the public Rekor instance - #2149

Open
fghanmi wants to merge 1 commit into
mainfrom
SECURESIGN-4026
Open

feat(console): use securesign Rekor URL instead of the public Rekor instance#2149
fghanmi wants to merge 1 commit into
mainfrom
SECURESIGN-4026

Conversation

@fghanmi

@fghanmi fghanmi commented Jul 27, 2026

Copy link
Copy Markdown
Member

No description provided.

@fghanmi
fghanmi marked this pull request as ready for review July 27, 2026 09:46
@qodo-for-securesign

Copy link
Copy Markdown

PR Summary by Qodo

console: configure UI to use SecureSign Rekor URL (replace public default)

✨ Enhancement ⚙️ Configuration changes 🕐 20-40 Minutes

Grey Divider

AI Description

• Add optional Rekor endpoint configuration to the Console UI spec.
• Propagate the configured Rekor URL into the Console UI Deployment env.
• Update CRD schema and generated deep-copies to support the new field.
Diagram

graph TD
  crd["Console CRD (.spec.ui.rekor)"] --> controller["Console controller"] --> util["apis.ServiceAsUrl()"] --> deploy["UI Deployment env"] --> pod["Console UI pod"] --> rekor[("Rekor endpoint URL")]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Auto-discover Rekor from managed Rekor CR/status
  • ➕ Zero user configuration for the common “operator-managed Rekor” case
  • ➕ Keeps Console CRD smaller and avoids duplicating endpoint config
  • ➖ Tighter coupling between Console and Rekor controllers/CRs
  • ➖ Requires selection logic when multiple Rekor instances exist
2. ConfigMap/env override (no CRD change)
  • ➕ Avoids CRD/schema changes and regeneration churn
  • ➕ Allows quick overrides without touching the Console spec
  • ➖ Weaker validation and discoverability vs typed CRD fields
  • ➖ More operational steps (create/manage ConfigMap) for users

Recommendation: The typed .spec.ui.rekor field is a reasonable approach for explicit, validated configuration and aligns with existing TasService patterns (ServiceAsUrl). Consider adding a defaulting behavior (or controller fallback) when Rekor address/port are unset, to avoid setting NEXT_PUBLIC_REKOR_DEFAULT_DOMAIN to an empty value.

Files changed (4) +25 / -0

Enhancement (2) +11 / -0
console_types.goAdd optional .spec.ui.rekor to Console API +3/-0

Add optional .spec.ui.rekor to Console API

• Introduces a new optional RekorService field under ConsoleUI to configure which Rekor instance the Console UI should use by default.

api/v1/console_types.go

deployment.goInject configured Rekor domain into Console UI Deployment +8/-0

Inject configured Rekor domain into Console UI Deployment

• Sets NEXT_PUBLIC_REKOR_DEFAULT_DOMAIN on the UI container using apis.ServiceAsUrl(instance.Spec.UI.Rekor), so the UI defaults to the configured Rekor endpoint instead of a public instance.

internal/controller/console/actions/ui/deployment.go

Other (2) +14 / -0
zz_generated.deepcopy.goDeep-copy support for ConsoleUI.Rekor +1/-0

Deep-copy support for ConsoleUI.Rekor

• Extends the generated ConsoleUI DeepCopyInto implementation to include the new Rekor field, keeping runtime object copying correct.

api/v1/zz_generated.deepcopy.go

rhtas.redhat.com_consoles.yamlExpose .spec.ui.rekor in the Console CRD schema +13/-0

Expose .spec.ui.rekor in the Console CRD schema

• Adds CRD OpenAPI schema for the new rekor block, including address and port fields with basic validation.

config/crd/bases/rhtas.redhat.com_consoles.yaml

@qodo-for-securesign

qodo-for-securesign Bot commented Jul 27, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Empty Rekor env override ✓ Resolved 🐞 Bug ≡ Correctness
Description
ensureUIDeployment always creates/sets NEXT_PUBLIC_REKOR_DEFAULT_DOMAIN from the optional
spec.ui.rekor, but Console defaults never populate that config and ServiceAsUrl returns an empty
string when address is unset. This makes the operator inject an explicit empty env var into the UI
pod, which can override the image/app default Rekor endpoint and break Rekor integration unless
users configure spec.ui.rekor.address.
Code

internal/controller/console/actions/ui/deployment.go[R121-126]

+		rekorURL := kubernetes.FindEnvByNameOrCreate(container, "NEXT_PUBLIC_REKOR_DEFAULT_DOMAIN")
+		url, err := apis.ServiceAsUrl(&instance.Spec.UI.Rekor)
+		if err != nil {
+			return err
+		}
+		rekorURL.Value = url
Relevance

●●● Strong

Team often accepts fixes preventing failures when optional service config is unset; similar
conditional URL resolution accepted.

PR-#1484
PR-#1723

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The PR introduces an unconditional env-var assignment using an optional service config that has no
defaults, and the URL helper can return an empty string without error, resulting in explicit empty
env var injection.

internal/controller/console/actions/ui/deployment.go[111-127]
api/v1/common.go[130-140]
api/v1/console_defaults.go[8-11]
internal/apis/tas_service.go[20-39]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The Console UI deployment always sets `NEXT_PUBLIC_REKOR_DEFAULT_DOMAIN` from `spec.ui.rekor`. When `spec.ui.rekor.address` is not provided (field is optional and defaults do not set it), `apis.ServiceAsUrl` returns an empty string and the operator injects an explicit empty env var, potentially overriding the UI image’s internal default.

## Issue Context
- `ConsoleUI.Rekor` is optional in the API/CRD, and `ConsoleUI.SetDefaults()` does not set it.
- `apis.ServiceAsUrl()` does not error on empty address and can return `""`.

## Fix Focus Areas
- internal/controller/console/actions/ui/deployment.go[118-127]
- api/v1/console_defaults.go[8-11]
- internal/apis/tas_service.go[20-39]

## Suggested implementation
- Only set `NEXT_PUBLIC_REKOR_DEFAULT_DOMAIN` when a non-empty URL can be derived (e.g., `if instance.Spec.UI.Rekor.Address != "" { ... }`).
- If Rekor is not configured, remove the env var from the container to preserve image defaults (use `kubernetes.RemoveEnvVarByName(container, "NEXT_PUBLIC_REKOR_DEFAULT_DOMAIN")`).
- Optionally, implement a defaulting/resolution behavior: if `spec.ui.rekor.address` is empty, resolve from a ready `Rekor` instance in the same namespace (similar to `internal/controller/tuf/utils/resolveURLFromService` patterns) and set the env var to that URL; otherwise leave it unset.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Qodo Logo

@codecov-commenter

codecov-commenter commented Jul 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.08%. Comparing base (3fa5719) to head (a6ba554).

Files with missing lines Patch % Lines
...ternal/controller/console/actions/ui/deployment.go 0.00% 12 Missing ⚠️
...ernal/controller/console/actions/api/deployment.go 0.00% 7 Missing ⚠️
api/v1/zz_generated.deepcopy.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2149      +/-   ##
==========================================
- Coverage   57.12%   57.08%   -0.05%     
==========================================
  Files         286      286              
  Lines       16129    16139      +10     
==========================================
- Hits         9214     9213       -1     
- Misses       5970     5980      +10     
- Partials      945      946       +1     
Flag Coverage Δ
e2e 69.77% <0.00%> (-0.01%) ⬇️
unit 35.65% <0.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@kdacosta0 kdacosta0 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we add at least a unit test for this function covering the case where spec.ui.rekor is set and when ommited?

ensureUIDeployment

Comment thread internal/controller/console/actions/ui/deployment.go Outdated
@osmman

osmman commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

@fghanmi can you try to rebase pr agains main. It could resolve problems in failing tests

@fghanmi
fghanmi force-pushed the SECURESIGN-4026 branch 3 times, most recently from 8d96340 to a1a513b Compare July 28, 2026 07:07
Comment thread api/v1/console_types.go Outdated
Ingress Ingress `json:"ingress,omitempty"`
// Rekor service configuration
//+optional
Rekor RekorService `json:"rekor,omitempty"`

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There is a new v1.ServiceReference object to handle dependencies between services. Can you please use it. @bouskaJ can provide more info/examples.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done, thanks!

apiURL := kubernetes.FindEnvByNameOrCreate(container, "CONSOLE_API_URL")
apiURL.Value = fmt.Sprintf("%s://%s:%d", scheme, apiHost, actions.ApiPort)

if instance.Spec.UI.Rekor.URL != "" {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Both spec.ui.rekor and spec.api.tuf use ServiceReference which accepts .ref or .url (mutually exclusive). Only .url is read by the controllers — .ref is silently ignored:

  • internal/controller/console/actions/ui/deployment.go:120 — reads .URL only
  • internal/controller/console/actions/api/deployment.go:50 — reads .URL only

A user setting .ref gets no error and no effect. The controller must resolve .ref by looking up the referenced CR's service URL.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@bouskaJ do you have some existing example/helper function to resolve url from ServiceReference.ref? I expect that there will have to be some logic to wait untill these services has know their ingress url.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yes, Does the Console supposed to consume public or internal URL? @fghanmi

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ideally, it should consumes public URL, but in "can" also use internal URL

@fghanmi fghanmi Jul 30, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

which means, could we use similar resolver on both TUF and rekor, just wondering

func init() {
	serviceresolver.Register(
		func(obj *rhtasv1.Tuf) (string, error) {
			return obj.GetServiceURL(), nil
		})
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Infra for public services is not yet merged (pending for @osmman re-review on #2126)

@fghanmi fghanmi Jul 30, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I added this commit 11d5d6a

Just an idea. It was tested fine.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We may use public address - the resolver for public address will be present once #2126 is merged.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

apiURL.Value = fmt.Sprintf("%s://%s:%d", scheme, apiHost, actions.ApiPort)

if instance.Spec.UI.Rekor.URL != "" {
rekorURL := kubernetes.FindEnvByNameOrCreate(container, "NEXT_PUBLIC_REKOR_DEFAULT_DOMAIN")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

When spec.ui.rekor.url is set, NEXT_PUBLIC_REKOR_DEFAULT_DOMAIN is added to the container. When the URL is later cleared or removed, the if block is skipped and no cleanup runs — the stale env var persists on the existing Deployment.

Example: User sets spec.ui.rekor.url: https://rekor.internal.corp → env var is created. Later they remove the field to switch back to the default public Rekor. The operator skips the if block but the old env var stays, so the UI keeps pointing at rekor.internal.corp — which may no longer exist.

Fix: Add an else branch with kubernetes.RemoveEnvVarByName(container, "NEXT_PUBLIC_REKOR_DEFAULT_DOMAIN") — same pattern used for SSL_CERT_DIR and SIGNER_PASSWORD elsewhere in the codebase.

…nstance

Signed-off-by: Firas Ghanmi <fghanmi@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants