Add support for Pro FIPS archives #61
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
woky
force-pushed
the
pro-new/fips
branch
2 times, most recently
from
7c935d2 to
49eb38d
Compare
woky
force-pushed
the
pro-new/fips
branch
2 times, most recently
from
c6d8dee to
4889166
Compare
Contributor
Author
|
The PR graph: |
woky
force-pushed
the
pro-new/fips
branch
7 times, most recently
from
88f4b4e to
7be552c
Compare
rebornplusplus
left a comment
rebornplusplus
left a comment
There was a problem hiding this comment.
It looks good to me overall. I have a few questions asking for more info.
woky
force-pushed
the
pro-new/fips
branch
2 times, most recently
from
1daec71 to
429a93c
Compare
cjdcordeiro
approved these changes
Jun 1, 2023
Collaborator
cjdcordeiro
left a comment
cjdcordeiro
left a comment
There was a problem hiding this comment.
my comments from #53 were addressed here so this lgtm
woky
force-pushed
the
pro-new/fips
branch
4 times, most recently
from
69b6e34 to
114d881
Compare
woky
force-pushed
the
pro-new/fips
branch
2 times, most recently
from
cdb5478 to
20e79cb
Compare
woky
force-pushed
the
pro-new/fips
branch
2 times, most recently
from
ec11f41 to
fcf970f
Compare
Member
TheSignPainter98
left a comment
TheSignPainter98
left a comment
There was a problem hiding this comment.
Just spotted some code-style issues and what I think are a few semantic oddities :)
| } | ||
| archives[archiveName] = openArchive | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
As the code block above populates archives and the block here checks it, I think they're strongly-linked enough to merit removing this blank line
| var ErrNoArchiveCredentials = errors.New("no archive credentials") | ||
|
|
||
| func initProArchive(pro string, archive *ubuntuArchive) error { | ||
| if i := sort.SearchStrings(validPro, pro); !(i < len(validPro) && validPro[i] == pro) { |
Member
There was a problem hiding this comment.
This is subjective, but to simplify the condition, would it be cleaner to use i >= len(validPro) || validPro[i] != pro?
Contributor
Author
|
@TheSignPainter98 Thank you very much for your suggestions. But as I said in #77 (comment), I have to postpone applying and responding to them until #60 is merged because of the way the PRs are structured. |
woky
force-pushed
the
pro-new/fips
branch
2 times, most recently
from
15ef5e9 to
127599b
Compare
woky
force-pushed
the
pro-new/fips
branch
3 times, most recently
from
3561514 to
3b11012
Compare
Currently we only test with the embedded base-files files package. This quite limits what we can test. But since commit a3315e3 ("testutil/pkgdata: Create deb programatically") we have the ability to define our own custom package content. Add support for testing with custom packages. These can be defined in each test case per archive name. The content can be defined directly in test case definition with the help of testutil.MustMakeDeb(). The package content is wrapped in the testPackage structure. This introduces one level of nesting in each test case package definition. In future, we will add package metadata to the package definition, namely version. So wrapping the package content in the structure now and later just adding a new field to it the structure later will make the future diff much smaller.
At some point we will need to get information about (deb) packages from (Go) packages other than archive, namely from the slicer package. For instance: - Package entries in Chisel DB will include version, digest and architecture. - Multi-archive package selection will need to know versions of a package in all configured archives. Add Info() method to the Archive interface that returns a new PackageInfo interface that contains accessors for the underlying control.Section of the package. Currently these accessors are Name(), Version(), Arch() and SHA256() methods. The reason to introduce PackageInfo interface instead of returing control.Section directly is keep the archive abstraction and not leak implementation details.
In the future, we'd like to support different kinds of archives which have a non-empty intersection of packages. In other words, we'd like to support archives that partly override the Ubuntu archive. Currently, each sliced package is tied to an archive via the "archive" property that defaults to the default archive. Drop this link from the sliced packages to the archives and select the latest available version of the package from all configured archives. This commit, in effect, doesn't change the current behavior because we don't allow archives to define their URLs. It's only preparation for future commits that will add support for different types of archives.
If a package to be downloaded exists in archive A and archive B, and archive A has higher priority than archive B, the package is downloaded from archive A even if archive B has a newer version. If two or more archives have the same priority, the most recent version of the package from all of them is selected, if any. Otherwise, lower priority archives are tried, if any.
When two or more archives with highest priority contain a package with the same version, the choice is effectively random, because the iteration order over options.Archives map is not defined. Get rid of this nondeterminism by sorting archives by their priority and then lexicographically by their labels (greater label wins) when the priorities are equal. Since archive labels are used as keys in YAML map, they should be unique and the order should be total.
Archive priority is currently defined as int. This can make chisel.yaml with very high/low priorities non-portable, e.g. from amd64 to arm32. Change archive priority definition to int32 data type.
Support Pro archives by a pro property in chisel.yaml. When the pro property is specified, it has to be either "fips" or "fips-updates". The repository URL is inferred from the value. Also, credentials for these URLs are searched. If credentials are not found, the corresponding pro archive is silently disabled. Otherwise, all requests to the archive are sent with the credentials found.
Collaborator
|
Per our offline discussion, I think our best way forward is to close this PR and discuss with @rebornplusplus to see if it is a priority right now, and how to amend the code here to make it compatible with the latest changes. We will take that discussion offline and we can always re-open the PRs or create new ones as we see fit. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Support Pro archives by a pro property in chisel.yaml. When the pro
property is specified, it has to be either "fips" or "fips-updates". The
repository URL is inferred from the value. Also, credentials for these URLs
are searched. If credentials are not found, the corresponding pro archive
is silently disabled. Otherwise, all requests to the archive are sent with
the credentials found.
This PR depends on #60 and #59