Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: Remove all traces of "identity.pub" from docs #699

Merged
merged 1 commit into from
Apr 29, 2022

Conversation

codablock
Copy link
Contributor

"identity.pub" is referenced multiple times in CRDs and docs. This secret
is however never used in any place. Instead, the public key is derived from
the "identity" private key.

This commit/PR removes all traces of "identity.pub", including from older
api versions.

Copy link
Member

@makkes makkes left a comment

Choose a reason for hiding this comment

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

I'm fine with removing it here but we'd need to remove it from flux/flux2, too. The identity.pub field is included in

  • create secret git
  • create source git
  • bootstrap git

@makkes makkes self-assigned this Apr 29, 2022
@makkes makkes added area/docs Documentation related issues and pull requests area/git Git related issues and pull requests area/security Security related issues and pull requests labels Apr 29, 2022
@hiddeco
Copy link
Member

hiddeco commented Apr 29, 2022

I'm fine with removing it here but we'd need to remove it from fluxcd/flux2, too.

That does not make sense to me, as we include it for sake of clarity when generating a private key for a user, and they e.g. need easy access to the public key later on (even if it can be derived from the PK).

@makkes
Copy link
Member

makkes commented Apr 29, 2022

I'm fine with removing it here but we'd need to remove it from fluxcd/flux2, too.

That does not make sense to me, as we include it for sake of clarity when generating a private key for a user, and they e.g. need easy access to the public key later on (even if it can be derived from the PK).

I kind of had the same thought. It's convenient to have the public in the secret for easy verification.

@stefanprodan
Copy link
Member

We've added the public key there, so people can extract it from the secret without needing openssh locally, just kubectl.

@hiddeco
Copy link
Member

hiddeco commented Apr 29, 2022

I would say this PR is still valid, because they are not required for the controller to function. However, the field should not be removed from the CLI commands.

@hiddeco hiddeco reopened this Apr 29, 2022
@codablock
Copy link
Contributor Author

I would agree on what @hiddeco says. The public key is not enforced by the controller, so it should not be documented as if it was. But having it for convenience is also a nice thing.

But requiring the user to add it manually still allows for error and make verification less reliable. It would require the controller to at least verify the public key matches the private key.

Or as an alternative: Let the git controller write the last observed public key to the status of the GitRepository. This way you can 100% know for sure that the seen public key matches the given private key, making manual verification much more reliable and easier.

Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

This looks good to me from a documentation perspective. For your suggestions around reflecting it in the Status, I think further discussion is required.

@hiddeco hiddeco removed the area/security Security related issues and pull requests label Apr 29, 2022
@hiddeco
Copy link
Member

hiddeco commented Apr 29, 2022

@codablock just discovered you likely changed docs/api/source.md by hand. It is generated through make api-docs however. Not sure why CI does not pick this up in the dirty Git state check, as running it locally gives me:

$ git diff
diff --git i/docs/api/source.md w/docs/api/source.md
index c5cec6f..c068591 100644
--- i/docs/api/source.md
+++ w/docs/api/source.md
@@ -1630,8 +1630,8 @@ Artifact
 <td>
 <code>includedArtifacts</code><br>
 <em>
-<a href="#source.toolkit.fluxcd.io/v1beta2.*github.com/fluxcd/source-controller/api/v1beta2.Artifact">
-[]*github.com/fluxcd/source-controller/api/v1beta2.Artifact
+<a href="#source.toolkit.fluxcd.io/v1beta2.*./api/v1beta2.Artifact">
+[]*./api/v1beta2.Artifact
 </a>
 </em>
 </td>

Copy link
Member

@makkes makkes left a comment

Choose a reason for hiding this comment

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

Otherwise lgtm

docs/api/source.md Outdated Show resolved Hide resolved
docs/api/source.md Outdated Show resolved Hide resolved
@codablock
Copy link
Contributor Author

codablock commented Apr 29, 2022

I've actually used make api-docs to update source.md. I just re-ran it on a vanilla main branch and the same change is performed. There seems to be something different on my machine compared to yours...not sure what it might be.

But I removed that change from the commit now.

@hiddeco
Copy link
Member

hiddeco commented Apr 29, 2022

@codablock what version of gen-crd-api-reference-docs do you have locally?

@codablock
Copy link
Contributor Author

@hiddeco 0.3.0, as downloaded by the Makefile. I also tried to downgrade to 0.2.0 just to ensure that it's not about the version, but that gave the same result. I've recently upgraded my golang installation to 1.18, maybe that's the reason. Unfortunately I don't have the time atm to verify if it would happen with 1.17 as well.

@makkes
Copy link
Member

makkes commented Apr 29, 2022

Running make api-docs doesn't result in any changes when I run it on main, neither with Go 1.18 nor with 1.17.9. I wonder what's happening...

@makkes
Copy link
Member

makkes commented Apr 29, 2022

btw, I filed an issue with gen-crd-api-reference-docs because the anchor and the text are wrong: ahmetb/gen-crd-api-reference-docs#49.

Not related to this PR, I just stumbled upon the bug by reviewing the PR.

@makkes
Copy link
Member

makkes commented Apr 29, 2022

@codablock feel free to cherry-pick makkes@929957d so you don't have to fix your setup in order to get this PR merged.

@codablock
Copy link
Contributor Author

@makkes Thanks. I though I reverted the changes already but somehow messed up the squash ¯_(ツ)_/¯
I squashed your commit into mine, so now it should be fine.

Copy link
Member

@makkes makkes left a comment

Choose a reason for hiding this comment

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

lgtm

@makkes
Copy link
Member

makkes commented Apr 29, 2022

@codablock please rebase your branch and force-push.

"identity.pub" is referenced multiple times in CRDs and docs. This secret
is however never used in any place. Instead, the public key is derived from
the "identity" private key.

This commit/PR removes all traces of "identity.pub" from v1beta2 CRDs and
docs.

Signed-off-by: Alexander Block <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docs Documentation related issues and pull requests area/git Git related issues and pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants