Skip to content
This repository was archived by the owner on Jul 11, 2023. It is now read-only.

Conversation

@keithmattix
Copy link
Contributor

@keithmattix keithmattix commented Jul 1, 2022

Description:
Creates a workflow for building and publishing FIPS-compliant versions of the OSM control plane. The modifications to the release process are in docs/release_guide.md.
Fixes #4888

Testing done:

Affected area:

Functional Area
New Functionality [X]
CI System [X]

Please answer the following questions with yes/no.

  1. Does this change contain code from or inspired by another project? no

    • Did you notify the maintainers and provide attribution? no
  2. Is this a breaking change? no

  3. Has documentation corresponding to this change been updated in the osm-docs repo (if applicable)? Not yet

@keithmattix
Copy link
Contributor Author

This version of OSM needs a custom version of Go (the Microsoft fork), so our CI/CD pipeline will need to be reworked to deploy this

@keithmattix keithmattix closed this Jul 5, 2022
@keithmattix keithmattix reopened this Jul 5, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jul 5, 2022

Codecov Report

Merging #4869 (257a8e3) into main (df502a7) will decrease coverage by 0.02%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##             main    #4869      +/-   ##
==========================================
- Coverage   68.40%   68.38%   -0.03%     
==========================================
  Files         218      218              
  Lines       15843    15844       +1     
==========================================
- Hits        10838    10835       -3     
- Misses       4950     4954       +4     
  Partials       55       55              
Flag Coverage Δ
unittests 68.38% <33.33%> (-0.03%) ⬇️

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

Impacted Files Coverage Δ
pkg/cli/environment.go 71.66% <ø> (ø)
pkg/cli/verifier/pod.go 52.55% <0.00%> (ø)
pkg/health/health.go 52.11% <0.00%> (ø)
pkg/webhook/server.go 0.00% <ø> (ø)
pkg/utils/mtls.go 100.00% <100.00%> (ø)
pkg/messaging/workqueue.go 89.28% <0.00%> (-10.72%) ⬇️
pkg/catalog/egress.go 86.19% <0.00%> (-0.42%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@keithmattix keithmattix marked this pull request as draft July 5, 2022 21:15
@keithmattix keithmattix force-pushed the fips-playground branch 2 times, most recently from 68fcbb9 to 197b1d8 Compare July 20, 2022 00:07
@keithmattix keithmattix marked this pull request as ready for review July 20, 2022 21:32
@keithmattix keithmattix requested a review from nshankar13 July 25, 2022 19:57
@keithmattix
Copy link
Contributor Author

@nshankar13 I'd love your POV to make sure that nothing needs to change for the nightly or openshift builds

@nojnhuh @shashankram I would really love your input around the changes to the CI and release processes.

- [Release Guide](#release-guide)
- [Release Candidates](#release-candidates)
- [Create a release branch](#create-a-release-branch)
- [Create release branches](#create-release-branches)
Copy link
Contributor

Choose a reason for hiding this comment

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

If we can parameterize the builds enough to build FIPS and non-FIPS from the same source and utilize that in CI and use something like a strategy.matrix to do both, I can see where we shouldn't need to change the release guide at all.

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We would still need to publish different tags/images, but in general I agree if we can parameterize this to that degree.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this file needs another pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah; I don't think there needs to be any changes in here after all right? Except maybe a note that ARM64 images will only be available via MCR

@nshankar13
Copy link
Contributor

@keithmattix I don't think we would need to change anything for the OpenShift job or the nightly-noinstall job, unless we want to run the FIPS tests nightly as well. In that case we can open a follow up PR to edit the nightly job and run the fips tests there.

@shashankram shashankram changed the title Fips playground fips: add workflow for building and publishing FIPS-compliant versions Jul 26, 2022
- [Release Guide](#release-guide)
- [Release Candidates](#release-candidates)
- [Create a release branch](#create-a-release-branch)
- [Create release branches](#create-release-branches)
Copy link
Member

Choose a reason for hiding this comment

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

+1

@nojnhuh
Copy link
Contributor

nojnhuh commented Jul 27, 2022

I played with parameterizing things and am fairly confident it will work. I have the changes limited to just osm-controller on a branch in my fork: main...nojnhuh:osm:fips-jon

  • Build FIPS images with make docker-build-osm-controller FIPS=1
    • FIPS=1 is just a convenience wrapper around setting CGO_ENABLED, the go build tag, and the base images for docker
  • The only hack is I've set up the dockerfile to run the yum install based on whether yum exists or not, but I don't see that causing issues unless people try to use other base images than the defaults.
  • I haven't tried making the CI changes yet, but I don't see any major roadblocks there. It just might be a bit messy to express "if $FIPS=1, then add '-fips' to the image tag" where we need to but that should be doable.

@keithmattix
Copy link
Contributor Author

@nojnhuh Thanks for that PoC; I haven't had to parameterize Dockerfiles like that before, so it was really helpful! The only blockers as far as CI workflows is this one. I don't think GitHub allows for conditional images. We could potentially create FIPS specific jobs within a workflow, and that may actually be the right direction to go if we can keep the release guide unchanged. What are your thoughts?

@nojnhuh
Copy link
Contributor

nojnhuh commented Jul 28, 2022

I wonder if a reasonable first step for CI would be to setup builds for FIPS images for releases and run e2e tests nightly, but skip all of the other stuff we do normally. I'd like to think the FIPS build of Go is close enough to the normal one that the risk of any difference breaking us is low. If we find a need to run more things more frequently, we can address the problem of how to configure that in CI then.

I don't think GitHub allows for conditional images.

I think the way we'd express that is by defining the image as part of the matrix.

@keithmattix
Copy link
Contributor Author

@nojnhuh that's a good point; I'm in favor of that direction. I'll get working on that

@keithmattix
Copy link
Contributor Author

CodeQL is complaining about InsecureTLSVerify, but the meat of the PR should be fixed. Let me know your thoughts @nojnhuh

Signed-off-by: Keith Mattix II <[email protected]>
Signed-off-by: Keith Mattix II <[email protected]>
Signed-off-by: Keith Mattix II <[email protected]>
Signed-off-by: Keith Mattix II <[email protected]>
Signed-off-by: Keith Mattix II <[email protected]>
Signed-off-by: Keith Mattix II <[email protected]>
Signed-off-by: Keith Mattix II <[email protected]>
Signed-off-by: Keith Mattix II <[email protected]>
Signed-off-by: Keith Mattix II <[email protected]>
Signed-off-by: Keith Mattix II <[email protected]>
Signed-off-by: Keith Mattix II <[email protected]>
Signed-off-by: Keith Mattix II <[email protected]>
Signed-off-by: Keith Mattix II <[email protected]>
Signed-off-by: Keith Mattix II <[email protected]>
Signed-off-by: Keith Mattix II <[email protected]>
Signed-off-by: Keith Mattix II <[email protected]>
Signed-off-by: Keith Mattix II <[email protected]>
Signed-off-by: Keith Mattix II <[email protected]>
Signed-off-by: Keith Mattix II <[email protected]>
Signed-off-by: Keith Mattix II <[email protected]>
@keithmattix keithmattix changed the title fips: add workflow for building and publishing FIPS-compliant versions fips: add workflow for building and publishing FIPS-compliant control plane Aug 9, 2022
@keithmattix keithmattix merged commit a51f453 into openservicemesh:main Aug 9, 2022
@keithmattix keithmattix deleted the fips-playground branch August 9, 2022 20:16
@keithmattix keithmattix restored the fips-playground branch August 10, 2022 00:31
@keithmattix keithmattix deleted the fips-playground branch January 17, 2023 18:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Build and publish a FIPS compliant OSM control plane

6 participants