Skip to content

Remove Azure Bucket public access from the docs and the code - #2138

Open
Pranav-error wants to merge 1 commit into
fluxcd:mainfrom
Pranav-error:docs-remove-azure-public-access
Open

Pranav-error wants to merge 1 commit into
fluxcd:mainfrom
Pranav-error:docs-remove-azure-public-access

Conversation

@Pranav-error

Copy link
Copy Markdown

Replaces #2137, which took the opposite approach. @stefanprodan and @matheuscscp both said on that PR that the docs should be corrected rather than the anonymous path restored, since no Bucket provider works without authentication — so this does that instead.

Refs #2136.

Docs

Three claims removed from docs/spec/v1/buckets.md:

  • "If no chain can be established, the bucket is assumed to be publicly reachable."
  • The azure-public example Bucket, which had no secretRef and relied on that behaviour.
  • "Note: that for a publicly accessible object storage, you don't need to provide a secretRef nor serviceAccountName."

That last one sat under the Service Account section and applied to all providers, not just Azure. I checked the others before removing it: gcp.NewClient uses a token source whenever no secret is present, and minio.NewClient always sets minioOpts.Creds. Neither has an anonymous path, which matches what you said. Say the word if you would rather I scope that removal to Azure only.

Code

The anonymous path was unreachable anyway. azureauth.NewTokenCredential never returns nil, so creds was never empty, chainCredentialWithSecret could not return the documented nil, nil, and azblob.NewClientWithNoCredential was dead. Removed the fallback, the dead nil return and the len(creds) > 0 guard so the code states the same thing as the docs.

Left alone, tell me if you want them

  • withoutCredentials() in blob.go is unexported and only called from blob_test.go, so it is an anonymous path reachable only by tests. It is unused in production either way; happy to remove it and adjust the tests in this PR or a follow-up.
  • Test_chainCredentialWithSecret still passes unchanged, since the chain is still always non-empty.

Verification

go build ./..., go vet ./internal/bucket/azure/ and go test ./internal/bucket/azure/ all clean.

Copilot AI lite review requested due to automatic review settings August 21, 2026 18:00

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@Pranav-error

Copy link
Copy Markdown
Author

Rebased onto current main — it had fallen ten commits behind. Neither file had been touched upstream, so the diff is unchanged.

Also brought the commit in line with AGENTS.md, which I had not followed: subject is now under 50 characters, the Refs #2136 moved out of the commit and into this description, and there is an Assisted-by trailer.

Verified against current main this time rather than by reading: go build ./internal/bucket/azure/... and go test ./internal/bucket/azure/... both pass.

One thing worth stating since it is easy to misread the diff: azblob.NewClientWithNoCredential still has two callers and both are correct. The withoutCredentials option at the top of NewClient is an explicit opt-in used by the tests, and the SAS path passes the token in the URL, so there is no credential to supply. What this removes is only the unreachable fallback at the end of the credential chain.

@matheuscscp

Copy link
Copy Markdown
Member

Hey @Pranav-error we agreed on changing only the docs

@Pranav-error

Copy link
Copy Markdown
Author

You are right, sorry — @stefanprodan's words on #2137 were about the docs and I took my own reply there as licence to remove the dead path too. Dropped it; this is docs only now.

Happy to send the code removal separately if you want it, or to leave it.

@Pranav-error
Pranav-error force-pushed the docs-remove-azure-public-access branch from 87e511e to e075e83 Compare September 18, 2026 17:15
Comment thread docs/spec/v1/buckets.md
Comment on lines -1007 to -1009
**Note:** that for a publicly accessible object storage, you don't need to
provide a `secretRef` nor `serviceAccountName`.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this note not relative to using provider: generic? It doesn't seem related to provider: azure. I think we can, instead of removing this note, improve it to be specific about provider: generic. A generic s3-like object storage can support public access. Please check if we have tests for public buckets with provider: generic.

@Pranav-error
Pranav-error force-pushed the docs-remove-azure-public-access branch from e075e83 to 145c526 Compare September 18, 2026 17:33
@Pranav-error

Copy link
Copy Markdown
Author

You are right on both counts — that note sits in the .spec.serviceAccountName section, directly after the bullets that separate generic from aws/azure/gcp, so it was never Azure-specific. I have restored it, scoped to generic:

Note: when .spec.provider is set to generic and the object storage allows anonymous access, you don't need to provide a secretRef nor a serviceAccountName. The cloud providers all require authentication.

generic does support it in code, unlike Azure: newGenericCreds returns nil when .spec.sts is unset, so minioOpts.Creds is nil and the client is built without credentials.

On tests: no, there are none for a public bucket with provider: generic. The closest is TestNewClientEmptySecret, which constructs a client with an empty Secret and asserts it is non-nil — it never fetches. Nothing in internal/ configures a public-read bucket policy on the mock (no SetBucketPolicy or equivalent anywhere), and every FGetObject/VisitObjects test authenticates. So the anonymous path for generic is documented but not covered.

Happy to add one — the in-process S3 mock from #2153 looks like the right place — but I would rather do that as its own PR than widen this one again.

None of the cloud Bucket providers work without authentication, and the
Azure anonymous path is unreachable in any case: azureauth.NewTokenCredential
never returns nil, so the credential chain is never empty and
azblob.NewClientWithNoCredential is not reached.

Drop the claim that a bucket with no credential chain is assumed to be
publicly reachable, and the azure-public example that relied on it.

Scope the note about public storage to provider: generic, which does
support anonymous access, rather than removing it.

Signed-off-by: sai pranav <rajasaipranav0@gmail.com>
Assisted-by: Claude Code/claude-sonnet-5
@Pranav-error
Pranav-error force-pushed the docs-remove-azure-public-access branch from 145c526 to f9874a1 Compare September 18, 2026 19:35
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.

3 participants