Fix TLS authority case sensitivity and kctrl error message grammar/formatting - #1863
Conversation
5ed8068 to
6fdcea9
Compare
joaopapereira
left a comment
There was a problem hiding this comment.
Looks good in general 1 issues that I think we should correct.
Another thing is Release note is accurate; optional: file a tracking issue (body invites that).
| // registry hostnames are case-insensitive per RFC 1035, and o.URL (freshly typed by the | ||
| // operator) and the stored Image (persisted by a possibly earlier invocation) are typed | ||
| // independently, so compare without regard to case | ||
| if strings.EqualFold(o.URL, existingRepository.Spec.Fetch.ImgpkgBundle.Image) && |
There was a problem hiding this comment.
Only the registry host is case-insensitive; org/repo path and tag can be case-sensitive (e.g. GHCR). Folding the whole string can treat distinct refs as a no-op. Prefer parse with name.ParseReference (already imported) and fold only the registry, or compare RegistryStr() case-insensitively and the remainder exactly.
There was a problem hiding this comment.
I've updated the implementation to use name.ParseReference() and now compare only the registry case-insensitively via Context().RegistryStr(), while comparing the repository path and tag case-sensitively. This prevents false matches when refs differ only in case.
6fdcea9 to
41dda28
Compare
…and repository add/update DNS/registry hostnames are case-insensitive per RFC 1035, but two comparisons in this codebase treated them as case-sensitive: - ShouldSkipTLSForAuthority compared the admin-configured dangerousSkipTLSVerify allow-list against the App/PackageRepository fetch URL's host using ==. A casing difference between the two independently-typed values silently left DangerousSkipTLSVerify false, causing kapp-controller to apply TLS verification opposite of the admin's intent. - kctrl's package repository add/update compared the --url flag against the persisted PackageRepository's stored image ref with ==, missing the no-op/idempotency short-circuit when only casing differed. Both now use strings.EqualFold. Added mixed-case regression cases to Test_ShouldSkipTLSForAuthority. Signed-off-by: Sameer <sameer.khan@broadcom.com>
- Fix "Expected to non-empty" → "Expected a non-empty" (namespace_flags.go) - Add missing spaces in error message formatting (secure_namespace_flags.go) - Update completion shell help text to include powershell (completion.go) - Fix error message capitalization and grammar (values_schema.go) - Standardize "non empty" → "non-empty" compound adjective usage (8 files) Signed-off-by: Sameer <sameer.khan@broadcom.com>
65a49d7 to
83816f4
Compare
Only the registry hostname is case-insensitive per RFC 1035. Repository paths and tags can be case-sensitive (e.g., GHCR). Use name.ParseReference() to properly parse both references when possible, comparing: - Registry case-insensitively via Context().RegistryStr() - Repository path and tag case-sensitively via Context().RepositoryStr() and Identifier() Falls back to original string comparison if parsing fails, maintaining backward compatibility for edge cases. This prevents treating distinct image references as duplicates when they differ only in repository/tag case. Signed-off-by: Sameer <sameer.khan@broadcom.com>
83816f4 to
f25ef08
Compare
|
@joaopapereira Thanks for the review! I've addressed the case-sensitivity issue with |
What this PR does / why we need it:
DNS/registry hostnames are case-insensitive per RFC 1035, but two comparisons in this codebase treated them as case-sensitive:
Config.ShouldSkipTLSForAuthority(pkg/config/config.go) compared the admin-configureddangerousSkipTLSVerifyallow-list against the host parsed from anApp/PackageRepository's fetch URL using==. Since the allow-list and the fetch URL are typed independently by different people (a cluster admin editing thekapp-controller-configSecret/ConfigMap vs. an app author writing a CR), a casing difference between the two (e.g.Registry.Example.Comvs.registry.example.com) silently leftDangerousSkipTLSVerifyfalse. The fetch then attempted real TLS verification against a host the admin explicitly intended to skip, failing with anx509: certificate signed by unknown authorityerror.kctrl package repository add/update(cli/pkg/kctrl/cmd/package/repository/add_or_update.go) compared the--urlflag against the already-persistedPackageRepository's stored image ref with==. A registry host typed with different casing than what's already stored missed the no-op/idempotency short-circuit, forcing an unnecessaryUpdate()call instead of just tailing status.Both comparisons now use
strings.EqualFoldinstead of==. Added mixed-case regression cases toTest_ShouldSkipTLSForAuthoritycovering both the bare-host and host:port branches.Additionally, this PR includes a set of grammar and formatting fixes to
kctrlCLI error and help messages, found via a targeted error-message audit:NamespaceNameFlag.resolveValue(cli/pkg/kctrl/cmd/core/namespace_flags.go): fixed the malformed error text"Expected to non-empty namespace name"→"Expected a non-empty namespace name".SecureNamespaceFlags.CheckForDisallowedSharedNamespaces(cli/pkg/kctrl/cmd/core/secure_namespace_flags.go): added missing spaces between concatenated string segments, which previously ran words together (e.g.namespace (%s)(hint: ...).kctrl completion(cli/pkg/kctrl/cmd/completion.go): the command'sShort/Longhelp text and itsdefaultcase error only listedbash, zsh or fishdespitepowershellbeing a supported and documented (ValidArgs,Example) shell; both now list all four shells, and the error message is capitalized consistently with otherkctrlerrors.PackageValuesSchemaParser.walkOnValueSchemaProperties(cli/pkg/kctrl/cmd/package/available/values_schema.go): fixed a missing space in the concatenated error string ("...has unsupported"+" type %v...") and capitalized the message for consistency."non empty"to the correct compound adjective"non-empty"across 8kctrlcommand files (app/delete.go,app/get.go,app/kick.go,app/pause.go,app/status.go,package/repository/kick.go,package/installed/{status,delete,create_or_update,get,pause_or_kick}.go).None of these are functional changes — they only affect the literal text of user-facing CLI error/help strings.
Which issue(s) this PR fixes:
Fixes #1864
Does this PR introduce a user-facing change?