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

ADR 1: add adr on autopilot basic auth + oras support #4524

Merged
merged 11 commits into from
Aug 27, 2024

Conversation

ricardomaraschini
Copy link
Contributor

Description

this pr adds an architecture decision record on adding oras and basic auth support on autopilot.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

How Has This Been Tested?

  • Manual test
  • Auto test added

Checklist:

  • My code follows the style guidelines of this project
  • My commit messages are signed-off
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

@ricardomaraschini ricardomaraschini requested a review from a team as a code owner May 30, 2024 15:00
@ricardomaraschini ricardomaraschini force-pushed the oras-plus-basic-auth-adr branch 3 times, most recently from b29ce58 to 5f0ae66 Compare May 30, 2024 16:25
@jnummelin
Copy link
Member

@ricardomaraschini you're missing sign-off for the commits

}
```

The secret pointed to by the provided `ArtifactPullSecret` property is expected to by of type `kubernetes.io/dockerconfigjson` if the protocol in use is `oci://` (see below) or of type `kubernetes.io/basic-auth` if protocols `http://` or `https://` are used .
Copy link
Member

Choose a reason for hiding this comment

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

Why only basic auth? If we'd have the full Authorization header value as a secret we'd be supporting basically everything, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was attempting to use only known secret types and there isn't one regarding a different kind of authentication.

It does not mean we can't introduce our Opaque thingy though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can change this proposal and add that if the secret contains an authorization entry we will send its content in the Authorization http header. What do you think ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I followed you. Let me know what you think of the updated proposal.

@ricardomaraschini ricardomaraschini force-pushed the oras-plus-basic-auth-adr branch 2 times, most recently from 89da778 to 82530a3 Compare June 4, 2024 09:37
@jnummelin jnummelin added this to the 1.31 milestone Jun 6, 2024
// ArtifactPullSecrets holds a reference to a secret where Docker or Basic Auth
// credentials are stored. We use these credentials when pulling the artifacts from
// the URL.
ArtifactPullSecret *ArtifactPullSecret `json:"artifactPullSecret,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Am I assuming correct that his would then apply to the existing http protocol too? If so, maybe write it up a bit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are correct. This is the change I have introduced, let me know if it suffices: 89335cb

Copy link
Member

@twz123 twz123 left a comment

Choose a reason for hiding this comment

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

Small personal formatting request: Would be cool if the markdown paragraphs were hard-wrapped to a fixed width.


No other property will be parsed and used. For sake of defining the expected behaviour in case of conflicting configurations:

> In the case where the three properties are set (`username`, `password`, and `authorization`) Autopilot will ignore `username` and `password`, i.e. `authorization` takes precedence over username and password.
Copy link
Member

Choose a reason for hiding this comment

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

What about using proper secret types for that, so there's no ambiguities?

There's already the well-defined Basic authentication Secret type. I'd just use that. We could add some k0s-specifc types, say auth.k0s.k0sproject.io/bearer-token (desugars into Authorization: Bearer ${data.token}) or auth.k0s.k0sproject.io/http-authorization-header (desugars into Authorization: ${data.authorization}).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My initial proposal was to use the Basic Authentication secret type, I diverted from it because someone mentioned it would be nice to support token based authentications as well.

In your opinion, what would be the advantage of defining two other types of Secrets on this specific case ?

I mean, one could define an Opaque secret with authorization: "Bearer abc123" while other could define it as authorization: "abc123" and we would have the same behaviour you described above, no ?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, in case of using the authorization header, it'd be easy enough for users to prefix their headers, if needed. There might still be some value in having a secret containing the "pure" credentials only, as the Bearer prefix is already protocol specific. If you want to use the same secret in other contexts, you'd maybe need to strip the "Bearer " prefix again before you can use it. Anyhow, only having one secret type for the header would be enough for now.

I'd really like to avoid the use of opaque secrets, as we then need to define which secret fields take precedence over others. Requiring explicit types from the beginning makes it much easier to add other auth types (e.g. for #4524 (comment)) in the future without having to implement, document and remember all the fallback rules.

When it comes to the `Opaque` secret layout (used for HTTP requests) Autopilot will accept the following entries:

- `username` and `password`: if both are set then Autopilot will attempt to pull the artifacts using [Basic Authentication](https://www.ibm.com/docs/en/cics-ts/6.1?topic=concepts-http-basic-authentication).
- `authorization`: if this property is set then the `Authorization` [header](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Authorization) will be set to its value when pulling the artifacts.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any possibility, that different registries might use different headers, e.g. X-Auth-Token, X-Access-Token, etc.? Or accept some additional headers?

Maybe we can have a secret type, that will put all the secret content directly into headers without any processing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my opinion we don't need to worry about this for registry access. Registries, more or less, work in the same way and we have implementation of registry clients for go. The authorization property here will be used only for HTTP or HTTPS requests.

@ricardomaraschini ricardomaraschini force-pushed the oras-plus-basic-auth-adr branch from b30b295 to 6319764 Compare July 3, 2024 19:20
Copy link
Contributor

github-actions bot commented Aug 2, 2024

The PR is marked as stale since no activity has been recorded in 30 days

@github-actions github-actions bot added the Stale label Aug 2, 2024
@ricardomaraschini
Copy link
Contributor Author

ricardomaraschini commented Aug 3, 2024

Can we somehow expedite this ? I can see that the automation is already tired of the silence here :-) I think I have already addressed all comments.

@github-actions github-actions bot removed the Stale label Aug 3, 2024

No other property will be parsed and used. For sake of defining the expected behaviour in case of conflicting configurations:

> In the case where the three properties are set (`username`, `password`, and `authorization`) Autopilot will ignore `username` and `password`, i.e. `authorization` takes precedence over username and password.
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, in case of using the authorization header, it'd be easy enough for users to prefix their headers, if needed. There might still be some value in having a secret containing the "pure" credentials only, as the Bearer prefix is already protocol specific. If you want to use the same secret in other contexts, you'd maybe need to strip the "Bearer " prefix again before you can use it. Anyhow, only having one secret type for the header would be enough for now.

I'd really like to avoid the use of opaque secrets, as we then need to define which secret fields take precedence over others. Requiring explicit types from the beginning makes it much easier to add other auth types (e.g. for #4524 (comment)) in the future without having to implement, document and remember all the fallback rules.

adds an adr on adding oras and basic auth support on autopilot.

Signed-off-by: Ricardo Maraschini <ricardo.maraschini@gmail.com>
addressing some issues reported on the adr.

Signed-off-by: Ricardo Maraschini <ricardo.maraschini@gmail.com>
as per pr comments it would be nice to also support the authorization
header. this commit covers this possibility to the adr.

Signed-off-by: Ricardo Maraschini <ricardo.maraschini@gmail.com>
be more explicit by mentioning the secret may be used for http as well
as for oci artifacts retrieval.

Signed-off-by: Ricardo Maraschini <ricardo.maraschini@gmail.com>
use InsecureSkipTLSVerify instead.

Signed-off-by: Ricardo Maraschini <ricardo.maraschini@gmail.com>
use an existing type instead of definining a new one.

Signed-off-by: Ricardo Maraschini <ricardo.maraschini@gmail.com>
Signed-off-by: Ricardo Maraschini <ricardo.maraschini@gmail.com>
Signed-off-by: Ricardo Maraschini <ricardo.maraschini@gmail.com>
@ricardomaraschini ricardomaraschini force-pushed the oras-plus-basic-auth-adr branch from fb9f7f8 to fff4dcc Compare August 5, 2024 13:53
as per pr review comment.

Signed-off-by: Ricardo Maraschini <ricardo.maraschini@gmail.com>
remove some dangling references to the old artifactPullSecret field.

Signed-off-by: Ricardo Maraschini <ricardo.maraschini@gmail.com>
if no protocol is provided (http, https, or oci) or if an invalid one is
provided an error should be signalled on the plan object.

Signed-off-by: Ricardo Maraschini <ricardo.maraschini@gmail.com>
Copy link
Member

@jnummelin jnummelin left a comment

Choose a reason for hiding this comment

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

@ricardomaraschini I see all the comments are resolved so I'm stamping this 👍 Thanks for your patience 😄

We can iron out the remaining details during implementation and update the ADR if needed.

@jnummelin jnummelin merged commit b504afe into k0sproject:main Aug 27, 2024
7 checks passed
@twz123 twz123 changed the title arch: add adr on autopilot basic auth + oras support ADR 1: add adr on autopilot basic auth + oras support Oct 10, 2024
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